-
Notifications
You must be signed in to change notification settings - Fork 166
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
Fix lists wrapping under the prefix #4702
Fix lists wrapping under the prefix #4702
Conversation
Sorry to hijack this, but is there a simple way to allow removing the border on the first item - with a utility maybe? |
I see two ways we could go about it, either add a modifier for the divided class In either case, I think it kind of weakens the pattern to allow for such customisation. I think we should decide if we want it always here or never. To reduce inconsistencies. Thoughts @bartaz @lyubomir-popov ? |
Let's not do it in this PR. I don't even know exactly what is expected. Do we always want to hide the first line? Only on nested lists? Why do we need it as an util? Who will be making a decision to use it or not. If we need it, let's make a separate issue of it and discuss details there. |
scss/_patterns_lists.scss
Outdated
margin-left: 0; | ||
} | ||
|
||
> * { |
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.
I'm a bit worried about assumptions we have to make about HTML and children elements in order for this to work. Like the *
is definitely a red flag if it can be avoided.
Would it work if we use padding or margin instead of "prefix" area, and position the prefix before
element absolutely on top of this padding/margin?
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.
Yeah you're right, I liked being able to cleanly define the spacing using the grid gaps but the inability to target the text content of the <li>
makes this approach a bit unreliable. Back to absolute positioning!
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.
LGTM.
I cleaned up couple things in the last commit @ClementChaumel if you wanna take a look what changed.
Done
Fix long list elements wrapping under the prefix (bullet, tick, etc.)
Fixes #4696
QA
Check if PR is ready for release
If this PR contains Vanilla SCSS 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 convention:Screenshots