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(data-table): sortable, boolean column, and overflow menu row fixes #6268

Merged

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Jun 12, 2020

Closes #6261
Closes #6262

This PR addresses data table issues specific to the boolean column, overflow menu, and sortable variants.

Testing / Reviewing

The row heights for all variants should be consistent with the headers, and the alignment and positioning of table elements should be consistent with the spec

@netlify
Copy link

netlify bot commented Jun 12, 2020

Deploy preview for carbon-elements ready!

Built with commit a523eb7

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

@netlify
Copy link

netlify bot commented Jun 12, 2020

Deploy preview for carbon-components-react ready!

Built with commit a523eb7

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

@tw15egan
Copy link
Collaborator

Small issue with some sortable / some not Data Table (Tall variant) in Safari.

All other variants look good 👍

Screen Shot 2020-06-15 at 1 59 20 PM

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.

Everything looks great to me ! thank you 🙌🏻

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Looks great after TJ's feedback 👀

@emyarod
Copy link
Member Author

emyarod commented Jun 23, 2020

just a heads up, the Safari issue is an unresolved flexbox issue at the browser level

compare this demo in different browsers: https://codesandbox.io/s/funny-golick-u5zfe

a workaround is to add a wrapper element around the table header content, but then we will need to adjust table styles across the board accordingly. if that's fine I can proceed with that plan

related links

https://codepen.io/scheinercc/pen/ZEGwpKb
https://bugs.webkit.org/show_bug.cgi?id=169700
https://bugs.webkit.org/show_bug.cgi?id=177503

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.

@emyarod I'm fine with it as is if it's a browser level issue, and it's only appearing on a very specific scenario (tall table with a mix of sortable headers, only on safari).

Thanks for the links!

@emyarod
Copy link
Member Author

emyarod commented Jun 23, 2020

I have the fix prepared if you want to take a look, but if we don't want to go forward with it I can just revert the latest commits

@emyarod emyarod force-pushed the 6262-data-table-inconsistencies branch from 6a46000 to cab0e5e Compare June 23, 2020 16:32
@tw15egan
Copy link
Collaborator

@emyarod ah gotcha, let me check it out

@tw15egan
Copy link
Collaborator

Nice! Looks like its rendering properly across all browsers. Didn't realize adding a span inside the button would get around the flex inconsistencies in buttons.

@emyarod emyarod force-pushed the 6262-data-table-inconsistencies branch from d58ccac to a523eb7 Compare July 1, 2020 20:53
@emyarod emyarod merged commit 5d79869 into carbon-design-system:master Jul 1, 2020
@emyarod emyarod deleted the 6262-data-table-inconsistencies branch July 1, 2020 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants