-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Horizontal list section component #5437
base: main
Are you sure you want to change the base?
Conversation
0192add
to
78dae55
Compare
quick chat-gpt based snippet to avoid partitioning into 3 without using the current grid at all /* Base styles for .row and its children / .row > * { /* Container query for widths < 33ch */ .row > * { /* Container query for widths >= 33ch and < 2*33ch (66ch) */ .row > * { /* Container query for widths >= 233ch (66ch) and < 433ch (132ch) */ .row > * { /* Container query for widths >= 4*33ch (132ch) */ |
(for easier copy-pasting: https://chatgpt.com/share/67618c6b-0668-8000-b0c6-079803ff13b7) |
78dae55
to
ce02120
Compare
Highlighting an issue with that chatgpt output: it seems it is suggesting an illegal use of container queries to change grid column template. .p-list--horizontal {
container-type: inline-size;
display: grid;
grid-template-columns: repeat(1, 1fr);
gap: 1rem;
.p-list__item {
grid-column: span 4;
}
@container (min-width: 33ch) {
// targeting the container itself within a container query is illegal
.p-list--horizontal {
grid-template-columns: repeat(4, 1fr);
.p-list__item {
grid-column: span 2;
}
}
}
@container (min-width: 66ch) {
.p-list--horizontal {
grid-template-columns: repeat(8, 1fr);
}
}
} Something that could work is wrapping the lists in some wrapper element: Intrinsic example with wrapper<div class="p-list--horizontal-wrapper">
<ul class="p-list--horizontal">
<li class="p-list__item is-ticked ">
<p>10 year security maintenance and CVE Patching</p>
</li>
<li class="p-list__item is-ticked">
<p>
<a href="#">Kernel Livepatch</a> for 24/7 patching with no downtime
</p>
</li>
<li class="p-list__item is-ticked">
<p>
<a href="#">Expanded security</a> for infrastructure and applications
</p>
</li>
<li class="p-list__item is-ticked">
<p>
<a href="#">FIPS 140-2</a> cryptographic modules certified by NIST
</p>
</li>
<li class="p-list__item is-ticked">
<p>
<a href="#">Common Criteria EAL2</a>: ISO/IEC IS 15408 validated by CSEC
</p>
</li>
<li class="p-list__item is-ticked">
<p>
<a href="#">DISA/STIG</a> hardening for <abbr title="Department of Defence, USA">DoD</abbr>
compliance
</p>
</li>
<li class="p-list__item is-ticked">
<p>
<a href="#">CIS profiles</a> for cyber defence and malware prevention
</p>
</li>
</ul>
</div> .p-list--horizontal-wrapper {
@extend %fixed-width-container;
container-type: inline-size;
.p-list--horizontal {
@extend %vf-list-with-state-icons;
display: grid;
column-gap: 1rem;
.p-list__item {
@extend %vf-list-item;
border-top: $border;
}
}
@container (width < 50ch) {
.p-list--horizontal {
grid-template-columns: repeat(1, 1fr);
}
}
@container (50ch <= width < 90ch) {
.p-list--horizontal {
grid-template-columns: repeat(2, 1fr);
}
}
@container (width >= 90ch) {
.p-list--horizontal {
grid-template-columns: repeat(4, 1fr);
// Skip the first column on large
&.is-3-cols-on-large {
.p-list__item {
&:nth-child(3n + 1) {
grid-column-start: 2;
}
&:nth-child(3n + 2) {
grid-column-start: 3;
}
&:nth-child(3n + 3) {
grid-column-start: 4;
}
}
}
}
}
} |
3e607a1
to
17617eb
Compare
|
||
### 25/75 Horizontal section | ||
|
||
You can also add the `.is-25-75` modifier to reserve 25% space at the start of the list and place the remaining items in the remaining 75% space. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmuzina Could it work without a modifier class name but nested inside row--25-75-on-large
or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do this. However, it would require the user to create an empty column, and wrap the list in a .col
as well, which I was trying to avoid to keep the markup very minimal.
Here's how that might look
<div class="row--25-75-on-large">
<div class="col"></div>
<!-- or omit the empty column above and use a col-start-large-3 here -->
<div class="col">
<ul class="p-list--horizontal-section">
<!--list items....-->
</ul>
</div>
</div>
Instead, I have gone for this:
<ul class="p-list--horizontal-section is-25-75">
<!--list items....-->
</ul>
scss/_patterns_lists.scss
Outdated
@@ -199,6 +216,49 @@ $list-step-bullet-margin: $sph--x-large; | |||
} | |||
} | |||
|
|||
@mixin vf-p-list-horizontal-section { | |||
.p-list--horizontal-section { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of how different it is from standard "lists", I wonder if making it a "variant" of p-list
is justified, or should it be more like it's own component (like p-horizontal-list-section
or something).
On the other hand, keeping it as p-list--
we can reuse existing p-list__item
and bullet/ticked options, so I guess there is a value in that consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this component behaves a bit like an HOC in that it is a rather featured combination of grid and list. However, it requires its own styling. So, I've implemented it as a list variant to be able to take advantage of other list classes and internal placeholders like you have mentioned.
17617eb
to
6492063
Compare
@lyubomir-popov Addressing your points:
This appears to be already occurring in production. Our visual testing is reporting that the only visual changes introduced here are the new list variant examples. Possibly we have some other issue to resolve related to split list baseline alignment, and we can create a separate issue for it. Let me know if that works for you.
I have synthesized our work from the pre-break pairing session with some polish of my own to make this even more robust (and avoid issues with page margin that we were seeing before). I set the border pseudoelement widths to If you're curious, the code is below: vanilla-framework/scss/_patterns_lists.scss Lines 223 to 293 in 6492063
|
can someone pls restart the demo? cc @bartaz |
There are some docker issues on the demos right now, will restart when its fixed |
@jmuzina looks great overall! A few nit picks:
|
53fd205
to
ab73aec
Compare
ab73aec
to
fa34b4b
Compare
@lyubomir-popov addressing your comments:
Fixed, using
Fixed, thanks.
|
Done
Implements the horizontal list section component per figma
Fixes #5433
QA
Check if PR is ready for release
If this PR contains Vanilla SCSS or macro code changes, it should contain the following changes to make sure it's ready for the release:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver conventionScreenshots
Screenshots enclosed
Default
Large
Medium
Small
25/75
Large
Medium
Small