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

[EuiBasicTable] Fix limited expandable row height #3855

Merged
merged 6 commits into from
Aug 18, 2020
Merged

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Aug 3, 2020

Summary

In some cases the max-height of a table row needs to be more than 1000px when expanded - for example when the table contains a large sub-table. I tried setting 100% { max-height: max-content}, but the expansion animation was not smooth, so instead added a two step animation -- to 1000px at 90%, and to max-content at 100%. I do not know if max-content is the right choice here, not an expert TBH.

Fixes #3854

Checklist

  • [ ] Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Safari and Firefox
  • [ ] Props have proper autodocs
  • [ ] Added documentation
  • [ ] Checked Code Sandbox works for the any docs examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3855/

2 similar comments
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3855/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3855/

@cchaos cchaos changed the title do not limit expandable rows in height [EuiBasicTable] Fix limited expandable row height Aug 18, 2020
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

🥇 Nice solution and thanks for the catch on this one.

This does cause a bit of an animation jank if the content reaches the 1000px height mark and then it's not longer a smooth animation to get to it's full height, but I think that's ok and hardly that noticeable with such a large max-height.

I did have a suggestion though to make that "jank" less visible which is to set the valued max-height to 100vh which should mean that the jank doesn't happen until the content is so tall it can't fit the full viewport height. Also, I think we can get even close to the 100% mark by changing 90% to 99%. I forked your original codesandbox with this solution and it seems to work well: https://codesandbox.io/s/nested-table-bug-forked-z232s

I also checked the browser support fo max-content and it looks good with only IE not supporting it which we basically just dropped support for anyway 👍

I updated your PR summary's checklist with items that need to be done to finish this PR up. Basically, it just really needs to be tested in a couple other browsers and a Changelog item added.

src/components/table/_table.scss Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3855/

@nyurik
Copy link
Contributor Author

nyurik commented Aug 18, 2020

Thanks @cchaos , I have added an entry to CHANGELOG, and tried your sandbox on Firefox 79 (Linux) and Android. I do not have access to Safari.

@nyurik nyurik requested a review from cchaos August 18, 2020 16:27
@cchaos
Copy link
Contributor

cchaos commented Aug 18, 2020

Looks like you might not have pushed you change to the Changelog?

@nyurik
Copy link
Contributor Author

nyurik commented Aug 18, 2020

@cchaos dope, thx, silly me, pushed it to my own repo instead of origin. Fixed and merged from master.

nyurik and others added 5 commits August 18, 2020 13:33
In some cases max-height of a sub-row needs to be more than 1000px. I tried setting `max-height: max-content`, but the expansion animation was not smooth, so instead added a two step animation -- to 1000px at 90%, and to `max-content` at 100%.

Fixes #3854
Thanks @cchaos , awesome suggestion!

Co-authored-by: Caroline Horn <[email protected]>
@cchaos
Copy link
Contributor

cchaos commented Aug 18, 2020

Eeek, it looks like we've got an issue in Safari. I don't think it's respecting that max-content. It animates open then shrinks back down essentially only becoming as tall as what the padding provides.

Screen Shot 2020-08-18 at 13 38 52 PM

Screen Shot 2020-08-18 at 13 40 01 PM

I'll try to play around some more with acceptable values for the 100% mark.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3855/

@nyurik
Copy link
Contributor Author

nyurik commented Aug 18, 2020

Sigh, seems like pretty soon both FF and safari will disappear due to all these incompatibilities... And chrome will be renamed into IE13 :). Thanks for catching it!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3855/

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

👍 Tested in Safari, Firefox, and Chrome. Nothing changes if the content is still smaller than the 100vh, but this is also a good solution if the consumer provides their own max-height for overflow too.

@nyurik nyurik merged commit 19961cd into master Aug 18, 2020
@nyurik nyurik deleted the nyurik-patch-1 branch August 18, 2020 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested tables do not properly expand
3 participants