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

fix(structured-list): top align header text #8166

Merged

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Mar 23, 2021

Closes #8157
related #721
related #753

This PR top aligns headers so that they stay aligned with non-header text in structured lists

Testing / Reviewing

Apply the header class to a list row cell and check its vertical alignment

@emyarod emyarod requested a review from a team as a code owner March 23, 2021 17:53
@netlify
Copy link

netlify bot commented Mar 23, 2021

Deploy preview for carbon-elements ready!

Built with commit c4c64b9

https://deploy-preview-8166--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Mar 23, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit c4c64b9

https://deploy-preview-8166--carbon-components-react.netlify.app

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@tw15egan
Copy link
Collaborator

tw15egan commented Mar 23, 2021

Should we check if the header is part of tbody, and if so change the line-height to match the rest of the row? It's currently getting the header line-height of 1.29 but the rest of the row has a 1.43 line-height

Screen Shot 2021-03-23 at 4 05 38 PM

@emyarod
Copy link
Member Author

emyarod commented Mar 24, 2021

the line heights are set by our type mixins so we will need to see if they need to be updated

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.

Looks good 🚀

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@github-actions
Copy link
Contributor

DCO Assistant Lite bot: Thanks for your submission! We ask that you all sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text:


I have read the DCO document and I hereby sign the DCO.


0 out of 2 committers have signed the DCO.
@emyarod
@kodiakhq[bot]
You can retrigger this bot by commenting recheck in this Pull Request

@kodiakhq kodiakhq bot merged commit 7a80e67 into carbon-design-system:main Mar 29, 2021
@emyarod emyarod deleted the 8157-structuredlist-row-headings branch March 30, 2021 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support _row_ headings for StructuredList
4 participants