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] Auto-fit rows to content #4958

Merged
merged 74 commits into from
Sep 21, 2021

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Jul 16, 2021

Closes: #4795

Summary

Adds 'auto' as value for defaultHeight from rowHeightsOptions which allow to rows auto fit to content.

@VladLasitsa VladLasitsa requested a review from alexwizp July 16, 2021 13:16
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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.

Couple questions to start with (some were made as comments in particular code locations)

I'm curious if you've tried a resize observer or similar to respond to changes in cell contents, instead of only during mount or onCellLoaded is called?

src/components/datagrid/data_grid_cell.tsx Outdated Show resolved Hide resolved
src/components/datagrid/row_height_utils.ts Outdated Show resolved Hide resolved
@VladLasitsa
Copy link
Contributor Author

VladLasitsa commented Jul 26, 2021

I'm curious if you've tried a resize observer or similar to respond to changes in cell contents, instead of only during mount or onCellLoaded is called?

You mean like this but for all cellContents?
image

I think mount, onCellLoaded and update height in shouldComponentUpdate cover all needs. But I can try to use ResizeObserver.

UPDATE: Start using ResizeObserver - work fine, also it allow to remove window.fonts.dowload method and now we setHeight in cache only here and in didUpdate method (needed because of sorting)

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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.

With the resize observer, these changes are easier to follow & my experience playing with the result seems to be better & more stable 👍

The scroll bar acts a little weird while scrolling. Without knowing all of the row heights, I don't think this behaviour is avoidable when scrolling down, but it seems that the grid loses its knowledge of row heights while scrolling back up.

datagrid_scrolling.mov

src/components/datagrid/data_grid_body.tsx Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_cell.tsx Outdated Show resolved Hide resolved
src/components/datagrid/row_height_utils.ts Outdated Show resolved Hide resolved
@VladLasitsa
Copy link
Contributor Author

but it seems that the grid loses its knowledge of row heights while scrolling back up.

Could you please provide a video or gif for this?

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor

When we change the density to the smaller size the last row on the screen gets overlapped by the footer (in fullscreen mode):

Screen.Recording.2021-09-16.at.03.06.PM.mov

But it seems this also happens in #/tabular-content/data-grid-virtualization:

Screenshot 2021-09-16 at 15 01 24

@kibanamachine
Copy link

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

@VladLasitsa
Copy link
Contributor Author

VladLasitsa commented Sep 16, 2021

So IMO we should set the toolbarVisibility={{ showStyleSelector: false }} for the examples we're providing. But it makes sense to have the Density aka toolbarVisibility={{ showStyleSelector: true }} for other use cases that uses auto. So we shouldn't disable it by default when auto is enabled.

Makes sense to me, but let's also be sure to explain that in the docs as well. Basically a sentence about when to keep/hide that density selector.

@VladLasitsa Can you push you visibility fix so we can test? It's hard to tell just by your screencast.

I pushed changes. @cchaos could you please take a look?

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

chandlerprall commented Sep 16, 2021

I tried the following solution: change visibility property in style prop - visibility: hidden when we don't have correct heights and visibility: visible when we reset grid after heights calculation. And now I have the following:

Data.grid.row.height.options.-.Elastic.UI.Framework.mp4
Is it okay?

If we try to fix in this PR (I'm good with Elizabet's recommendation to solve it separately),

@VladLasitsa Cell from data_grid_body.tsx line 52 receives an isScrolling prop that we could probably base that on:

Adds an additional isScrolling parameter to the children render function. This parameter can be used to show a placeholder row or column while the list is being scrolled.

Note that using this parameter will result in an additional render call after scrolling has stopped (when isScrolling changes from true to false).

https://react-window.vercel.app/#/api/FixedSizeGrid

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

Let's resolve the flashing content in a separate PR; I think it'd be good to do some more investigation into the issue & solutions first, and don't want to hold auto-fitting up for that.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor

@cchaos, @VladLasitsa, and @chandlerprall,

I updated both height demos and the density is now working. To make it work I used relative font sizes in EuiText and EuiMarkdownFormat.

density@2x

I also tried to explain better how the font size overriding works. Feel free o suggest a better explanation.

Screenshot 2021-09-20 at 19 27 48

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

I finished the design review. Tested in Chrome, Safari, Edge and Firefox, and LGTM. 🎉

The only issue I can still find is this one: #4958 (comment). I was not able to solve it.

And thanks @VladLasitsa for all the effort and patient. This feature looks amazing! 👍🏽

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.

Thank you @VladLasitsa for so much effort on this project, and everyone else's fantastic attention to detail in reviews & additional contributions. Y'all are awesome ❤️

This looks ready to merge, I'll make a new issue to track #4958 (comment) as it exists on master separate from these changes.

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.

[EuiDataGrid] Expose prop to control number of prefetched rows for virtualized scrolling