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): update position of th cells to preserve border in Firefox #4822

Merged
merged 12 commits into from
Dec 6, 2019
Merged

fix(data-table): update position of th cells to preserve border in Firefox #4822

merged 12 commits into from
Dec 6, 2019

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Dec 5, 2019

Partially resolves style issue 5 identified in #2330

If you look at the "selection" data table 🚨 in Firefox 🚨, there is something off about the first th (checkbox) and last th (:last-of-type) -- http://react.carbondesignsystem.com/?path=/story/datatable--with-selection

There's a 1px overflow when nothing is selected...
Screen Shot 2019-12-05 at 11 02 47 AM

And then no border when items are batch selected...
Screen Shot 2019-12-05 at 10 27 14 AM

So this PR proposes changing the position of those th elements from position: relative to the default, position: static. I explicitly changed it to static, rather than just removing the rule, because I think it's useful in this case to be explicit + include a note so anyone who comes along later understands the reasoning.

Changelog

Changed

  • change position to static for checkbox th and th:last-of-type in heading row

@jendowns jendowns requested a review from a team as a code owner December 5, 2019 17:12
@ghost ghost requested review from aledavila and dakahn December 5, 2019 17:12
@netlify
Copy link

netlify bot commented Dec 5, 2019

Deploy preview for the-carbon-components ready!

Built with commit 9a412f8

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

@netlify
Copy link

netlify bot commented Dec 5, 2019

Deploy preview for carbon-components-react ready!

Built with commit 9a412f8

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

@netlify
Copy link

netlify bot commented Dec 5, 2019

Deploy preview for carbon-elements ready!

Built with commit 9a412f8

https://deploy-preview-4822--carbon-elements.netlify.com

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.

Thanks @jendowns for jumping in! One question; Did you see there is no element with position:absolute in those table columns? I'm asking this PR seems to change the position calculation logic of such position:absolute elements if there is.

@jendowns
Copy link
Contributor Author

jendowns commented Dec 5, 2019

Hey @asudoh -- no, I didn't see absolutely-positioned descendants of these cells. Just text and icons 👍 (EDIT: but to clarify, I'm only testing use cases I'm aware of, namely those shown in the storybook... do you know the purpose of the position: relative being added on these elements originally?)

@asudoh
Copy link
Contributor

asudoh commented Dec 5, 2019

I see, the history shows #2289, so if you can quickly test it's not broken I think we can go forward!

@jendowns
Copy link
Contributor Author

jendowns commented Dec 5, 2019

Hey @asudoh, haha yes the first thing I did was check the git history 😅 The specific commits associated with these lines appear to all be related to "motion" updates, but again: there's no absolutely positioned descendants that I can see.

What I meant by my edited comment above: I'm only aware of the use case I see in the storybook. Is there another circumstance, where an absolutely positioned element maybe be added to either th cell, that I'm not aware of?

Thank you!

@asudoh
Copy link
Contributor

asudoh commented Dec 5, 2019

Thanks @jendowns - To your question I don't have a good idea. If we want to dig more about this area... My question would be if you have more details on why you thought position:relative is the culprit?

@jendowns
Copy link
Contributor Author

jendowns commented Dec 5, 2019

Oh I see what you mean @asudoh --

Personally anytime I see position: relative on a table child element, I start to look closer.
Specifically I'm thinking of this detail (look under 'position' > relative headings) -- https://www.w3.org/TR/CSS21/visuren.html#propdef-position

The effect of 'position:relative' on table-row-group, table-header-group, table-footer-group, table-row, table-column-group, table-column, table-cell, and table-caption elements is undefined.

And if I'm not mistaken, I don't believe the spec has been updated for position since then.

@jendowns
Copy link
Contributor Author

jendowns commented Dec 6, 2019

Thank you @joshblack I just updated the formatting of my comments!
Also was curious if you thought the comments were sufficient? Based on the discussion here, I wonder if I shouldn't include more info on the spec (see: #4822 (comment))?

Maybe something that references less of one "effect" (i.e., "Required to preserve borders") and more of the cause? Like this:

// Do not use `position: relative`, as its behavior is undefined: https://www.w3.org/TR/CSS21/visuren.html#propdef-position

Too much?

@joshblack
Copy link
Contributor

@jendowns always down to include more info over less, to be honest, but this is 100% my personal preference haha. Especially if it's something crazy like this.

@joshblack
Copy link
Contributor

One quick note to reviewers, this is observable in Firefox 👍

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! Would love to see your additional comments too, @jendowns if you wanted to add them

@jendowns
Copy link
Contributor Author

jendowns commented Dec 6, 2019

Thank you @joshblack! Just updated the comment to include those details 👍

@jendowns jendowns changed the title fix(data-table): update position of th cells to preserve border fix(data-table): update position of th cells to preserve border in Firefox Dec 6, 2019
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.

💯 explanation - Thanks @jendowns!

Copy link
Contributor

@aledavila aledavila left a comment

Choose a reason for hiding this comment

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

Thanks Jen. Good PR

@asudoh asudoh merged commit f5325f7 into carbon-design-system:master Dec 6, 2019
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.

None yet

5 participants