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: add line height to unsortable columns on sortable datatable (#9311) #11404

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

pmweeks98
Copy link
Contributor

@pmweeks98 pmweeks98 commented May 12, 2022

Closes #9311

Changed

Added lineheight to ensure the correct font size is used in unsortable columns

Testing / Reviewing

{{ Add descriptions, steps or a checklist for how reviewers can verify this PR works or not }}

@pmweeks98 pmweeks98 requested a review from a team as a code owner May 12, 2022 15:35
@github-actions
Copy link
Contributor

github-actions bot commented May 12, 2022

DCO Assistant Lite bot All contributors have signed the DCO.

@pmweeks98
Copy link
Contributor Author

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

@pmweeks98
Copy link
Contributor Author

recheck

@netlify
Copy link

netlify bot commented May 12, 2022

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ea3d24e
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/627d295951ad0600097515ba
😎 Deploy Preview https://deploy-preview-11404--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented May 12, 2022

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit ea3d24e
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/627d29595ea91200082d96ff
😎 Deploy Preview https://deploy-preview-11404--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented May 12, 2022

Deploy Preview for carbon-components-react ready!

Name Link
🔨 Latest commit 32018bd
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/62ac9655ce02cb000980f323
😎 Deploy Preview https://deploy-preview-11404--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented May 12, 2022

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 32018bd
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/62ac9655025b200009f18769
😎 Deploy Preview https://deploy-preview-11404--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@tay1orjones
Copy link
Member

@pmweeks98 could you share a link to the storybook where the original bug can be reproduced to compare with the fix made here? I'm having a hard time determining what/where this fixes the column headers.

@jnm2377
Copy link
Contributor

jnm2377 commented May 12, 2022

@tay1orjones this is the repro I saw in the issue... https://codesandbox.io/s/happy-minsky-n5i5s?file=/index.html
but I think it's using vanilla so I'm not sure if it's the same for react.

@tay1orjones
Copy link
Member

@jnm2377 Oh thanks! I missed that. Yeah I can't repro in react. If this passes the VRT checks then I think it's alright?

@pmweeks98
Copy link
Contributor Author

@tay1orjones can you advise on how to get a storybook with my changes in it?

@tay1orjones
Copy link
Member

@pmweeks98 To edit the storybook you can modify the data table .stories.js files. When a commit is pushed to this PR the storybook will be deployed to a PR deploy preview via netlify in the comment/links above.

@pmweeks98
Copy link
Contributor Author

@tay1orjones @jnm2377 have updated storybook to have a table with unsortable columns on sortable table, table size is set to xl now, not sure if that is desired, can ye have a look?

@tay1orjones
Copy link
Member

@pmweeks98 Cool! Yeah I can see the fix when I toggle the line-height off in the dev tools. They don't align correctly and then with the fix turned on it aligns exactly as it should.

aligns.properly.with.the.line.height.applied.mov

Thanks for adding the story to demo the fix. I think in this case we can probably remove it now since there isn't much of a need for a story demonstrating this beyond to validate this bug. Once removed this looks good to me to merge 👍

@pmweeks98 pmweeks98 force-pushed the pweeks/9311_v2 branch 2 times, most recently from df2d1a2 to d753cef Compare June 3, 2022 09:32
@pmweeks98
Copy link
Contributor Author

@tay1orjones removed

@pmweeks98
Copy link
Contributor Author

@jnm2377 ping

@joshblack joshblack merged commit 1134fde into carbon-design-system:main Jun 17, 2022
@pmweeks98 pmweeks98 deleted the pweeks/9311_v2 branch June 20, 2022 08:06
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.

Vertical alignment of sortable columns vs. non-sortable columns in XL tables
4 participants