Skip to content
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

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

NotWearingPants
Copy link
Contributor

@NotWearingPants NotWearingPants commented Oct 23, 2021

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)
image

Closes

Fixes #1130

@lubber-de lubber-de added state/awaiting-reviews Pull requests which are waiting for reviews lang/css Anything involving CSS labels Oct 23, 2021
@lubber-de lubber-de added this to the 2.9.0 milestone Oct 23, 2021
@lubber-de lubber-de added the type/bug Any issue which is a bug or PR which fixes a bug label Oct 23, 2021
Copy link
Member

@lubber-de lubber-de left a 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:
image
To fix this, you should remove the following code, because it is not needed by your translatex(-100%) approach anymore (as the variable @orderedChildCountOffset is then not used, you can also remove it from list.variables OR simply set the variable to 0.65rem as well (somehow more backward compatible if people might have changed it in their custom themes)..

ol.ui.list ol li:before,
.ui.ordered.list .list > .item:before {
margin-left: @orderedChildCountOffset;
}

See https://jsfiddle.net/lubber/L8f627oy/11/
image

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;
}

See https://jsfiddle.net/lubber/L8f627oy/24/

@lubber-de
Copy link
Member

@NotWearingPants Are you interested in continuation to work on this PR? Please see my review 😉

@NotWearingPants
Copy link
Contributor Author

@lubber-de I am, but finding the time isn't always easy :) I'mma try now

@lubber-de lubber-de added the state/awaiting-changes Anything which is awaiting changes label Nov 9, 2021
@lubber-de
Copy link
Member

@NotWearingPants Do you think you can work on this again?

@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Jan 6, 2022
@lubber-de lubber-de modified the milestones: 2.9.0, 2.9.x Jan 6, 2022
@lubber-de lubber-de added state/awaiting-changes Anything which is awaiting changes state/on-hold Issues and pull requests which are on hold for any reason state/awaiting-response Issues or pull requests waiting for a response and removed state/awaiting-changes Anything which is awaiting changes labels Feb 1, 2022
@lubber-de
Copy link
Member

@NotWearingPants Maybe this got forgotten: Do you think you can work on this again? Please see my review

@lubber-de lubber-de marked this pull request as draft August 2, 2023 07:13
@ko2in
Copy link
Member

ko2in commented Oct 10, 2023

@lubber-de It's been too long for not replying. Should I take over or would you like to finish with your reviews?

@lubber-de
Copy link
Member

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS state/awaiting-response Issues or pull requests waiting for a response state/on-hold Issues and pull requests which are on hold for any reason type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ordered and suffix list styles bug after 9.
3 participants