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.

Sign up for GitHub

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

Jump to bottom

fix(list): correct list alignment and numbering #4485

Merged
merged 6 commits into from
Oct 30, 2019
Merged

fix(list): correct list alignment and numbering #4485

merged 6 commits into from
Oct 30, 2019

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Oct 29, 2019

Closes #4433
Closes #3133

This PR adjusts ordered and unordered list spacing according to the updated spec and resolves a spacing issue for ordered lists with many list items. Previously we were using CSS counters and pseudo elements to replace the ordered list numbers, but since the spacing of these numbers was fixed they would overlap with the list item content if the number became large. We are no longer using pseudo elements for ordered lists and rely on the default implementation for ordered list numbering and spacing. (This may cause downstream changes for the Gatsby theme, website, and IDL site ref carbon-design-system/design-language-website#229, carbon-design-system/gatsby-theme-carbon#189)

Changelog

Changed

  • update horizontal and vertical spacing according to updated component spec
  • fix invalid markup in component examples

Removed

  • CSS counter for ordered list numbering

Testing / Reviewing

Ensure the components match the latest spec (ordered, unordered, nested, etc)

Ensure that no content overlaps with the numbers for long ordered lists

@emyarod emyarod requested a review from a team as a code owner October 29, 2019 19:30
@ghost ghost requested review from aledavila and jnm2377 October 29, 2019 19:31
@emyarod emyarod requested review from joshblack and laurenmrice and removed request for aledavila and jnm2377 October 29, 2019 19:31
@netlify
Copy link

netlify bot commented Oct 29, 2019 •
edited
Loading

Deploy preview for carbon-elements failed.

Built with commit 43c42b3

https://app.netlify.com/sites/carbon-elements/deploys/5dba141aeac0160008ad8df5

@netlify
Copy link

netlify bot commented Oct 29, 2019 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit 43c42b3

https://deploy-preview-4485--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Oct 29, 2019 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit 43c42b3

https://deploy-preview-4485--the-carbon-components.netlify.com

joshblack
joshblack reviewed Oct 29, 2019
View reviewed changes
packages/components/src/components/list/_list.scss Outdated
}

.#{$prefix}--list--nested {
margin-bottom: rem(4px);
margin-left: $carbon--spacing-07;
margin-top: #{$carbon--spacing-02};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these values need to be interpolated like this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I think I carried it over from the old code and copied it in several places

packages/components/src/components/list/_list.scss
&::before {
position: absolute;
left: -#{$carbon--spacing-05};
content: '\002013'; // – en dash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment here 🙏

@joshblack joshblack requested a review from a team October 30, 2019 18:23
@ghost ghost requested review from aledavila and asudoh and removed request for a team October 30, 2019 18:23
laurenmrice
laurenmrice approved these changes Oct 30, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

asudoh
asudoh reviewed Oct 30, 2019
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM basically, just one thing to double-check - Does existing list markup work with the new style? Thanks!

@emyarod
Copy link
Member Author

emyarod commented Oct 30, 2019 •
edited
Loading

as long as the correct classes are applied (ul gets .bx--list--unordered, ol gets .bx--list--ordered, and any nested list also gets .bx--list--nested), the styles will be backwards compatible

previously in our docs we were inconsistent about which uls and ols received the class name so we ended up with increased specificity for several selectors

@asudoh
Copy link
Contributor

asudoh commented Oct 30, 2019

OK let me change my question a bit... When an existing application upgrades carbon-components without changing their markup, would we expect sudden change in UI? If so, is your thoughts on that any such breakage are from some mistake in CSS usage in their markup?

@emyarod
Copy link
Member Author

emyarod commented Oct 30, 2019

yes, and the solution would be to ensure the classes are consistently applied in the list (every ul has .bx--list--unordered, every ol has .bx--list--ordered, and if the list is nested then it also receives .bx--list--nested)

asudoh
asudoh approved these changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks a lot @emyarod for clarifying!

@asudoh asudoh merged commit ac7fe05 into carbon-design-system:master Oct 30, 2019
@emyarod emyarod deleted the 4433-list-item-spacing branch October 31, 2019 14:47
@abbeyhrt abbeyhrt mentioned this pull request Nov 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurenmrice laurenmrice laurenmrice approved these changes

@aledavila aledavila aledavila approved these changes

@joshblack joshblack joshblack approved these changes

@asudoh asudoh asudoh approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[list] need to add 4px spacing between each item Unordered list breaks if 10 or more items
5 participants
@emyarod @asudoh @joshblack @aledavila @laurenmrice