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

[EuiDataGrid] Fix open cell popovers causing auto height rows to bug out #5618

Closed

Conversation

cee-chen
Copy link
Member

Summary

This was introduced in #5550 by our conversion to EuiWrappingPopover, which now dynamically inserts 2 extra DOM nodes around the cell popover anchor wrapper only when the popover is open:

For some bizarre reason, this triggers the cell content's offsetHeight to briefly be larger than it should be, which immediately updates the row height cache even though the cell content height returns to normal almost immediately after.

The easiest solution I could think of was to avoid setting the row height cache when the popover is opening, but I'm not sure if this will have unintentional side effects on consumer updates when the user has the popover open - those are likely very edge case, fingers crossed. I think the absolute best long-term solution might be to try and make EuiWrappingPopover/EuiPopover optionally not add DOM wrappers, but this might be the best fix in the interim, unless you can think of something else.

Really hoping we can get this merged in / backported along with the type fixes for the next Kibana upgrade! 🙏

Before

before

After

after

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for any docs examples

- [ ] 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_5618/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

This works for now to get a fix out, the root cause is coming from the wrapped portal's dom node being injected next to the cell content, cutting the width in half which can scale down content (e.g. images). Let's chat tomorrow and I'll show you what I've tracked down, and where I'm getting confused with trying to debug all these DOM nodes.

@cee-chen
Copy link
Member Author

Definitely super happy to pair on this and try to figure out what the heck is going on haha. It sounds like what you're saying is that maybe flexbox is the issue? Super interesting, I hadn't thought of that!

EDIT: Yup, I just turned off display: flex on the wrapping .euiDataGridRowCell and that also appeared to fix the issue 🤦‍♀️ Hot dang. Should I maybe use a conditional class approach for auto height instead in that case?...

@cee-chen
Copy link
Member Author

Closing this in favor of #5622 - after having done more cross-browser testing, I'm confident it's the better direction to go in

@cee-chen cee-chen closed this Feb 10, 2022
@cee-chen cee-chen deleted the datagrid/fix-auto-height-popover branch February 10, 2022 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants