-
-
Notifications
You must be signed in to change notification settings - Fork 333
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
[list] fix styling of multi-digit ordered lists (#1130) #2142
base: develop
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR. I changed your jsfiddle to use the same selector as used by the patch itself.
https://jsfiddle.net/lubber/L8f627oy/9/
You will recognize that nested ordered lists are not properly positioned anymore:
To fix this, you should remove the following code, because it is not needed by your translatex(-100%) approach anymore (as the variable set the variable to 0.65rem as well (somehow more backward compatible if people might have changed it in their custom themes)..@orderedChildCountOffset
is then not used, you can also remove it from list.variables
OR simply
Fomantic-UI/src/definitions/elements/list.less
Lines 737 to 740 in 67babf5
ol.ui.list ol li:before, | |
.ui.ordered.list .list > .item:before { | |
margin-left: @orderedChildCountOffset; | |
} |
See https://jsfiddle.net/lubber/L8f627oy/11/
Also, such kinda long lists now do not have a nice left padding space anymore (at least when the counter reaches 100), so i suggest 2 additional changes
- Change the suffixed list margin left from 1.25rem to 1.5rem (because the dot moves even single numbers too much to the left otherwise) ONLY to the first list element (not for nested ones)
- Invent a new
long
variant which further increases the left margin to 2rem, again ONLY to the first list element (not for nested ones)
ol.ui.suffixed.list,
.ui.suffixed.ordered.list,
ol.ui.suffixed.list ol {
margin-left: 1.5rem;
}
ol.ui.long.list,
.ui.ordered.long.list,
ol.ui.long.list ol {
margin-left: 2rem;
}
@NotWearingPants Are you interested in continuation to work on this PR? Please see my review 😉 |
@lubber-de I am, but finding the time isn't always easy :) I'mma try now |
@NotWearingPants Do you think you can work on this again? |
@NotWearingPants Maybe this got forgotten: Do you think you can work on this again? Please see my review |
@lubber-de It's been too long for not replying. Should I take over or would you like to finish with your reviews? |
My review was done already, so if you like you can take over |
Description
Fixes styling of multi-digit ordered lists - made the number aligned to the right of the text, so that when more digits are required they will be added on the left and the right edge of the number will stay in place.
I used
translateX(-100%)
to keep the right edge in a constant position, and adjusted the margin a bit since it previously accounted for the width of the number itself. The number I adjusted it to is to keep the look of numbers with single digits the same.Testcase
https://jsfiddle.net/futybd2v/
Screenshot
Current (left) vs fixed (right)
Closes
Fixes #1130