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

[Datagrid] Fix rowHeightOption: auto datagrid calculation issue #8251

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kqualters-elastic
Copy link
Contributor

@kqualters-elastic kqualters-elastic commented Dec 19, 2024

Summary

Resolves #8245

The useUnconstrainedHeight hook used by datagrid has I think a subtle bug in it around auto height rows, in cases where the row height is 'auto' for all the rows in the table, and when datagrid is aware of them early in the multiple renders it uses to settle on a height, the returned value of this hook, an integer representing the height of the datagrid if it were rendered completely in full is returned as a number based on just the header row height, as described in the issue above. The reason for this, I believe, is because RowHeightUtils.getRowHeightOption will return not always a number, but sometimes the string 'auto' as well. That causes the if at https://github.com/elastic/eui/blob/main/packages/eui/src/components/datagrid/utils/grid_height_width.ts#L137 to be true, and as this is the first time we are attempting to determine the height of this row, the heightsCache of the RowHeightUtils class (these really desperately need to be converted to hooks, and a lot of these problems will disappear) returns 0 for all rows when calling getRowHeight, but we increment the knownRowCount anyway, and the result is this bug. The fix in this PR seems to be the most minimal way to fix this, however I think refactoring the class based utils that revolve around EuiDataGridRowHeightOption/EuiDataGridRowHeightsOptions would be best long term, as now multiple unneeded re-renders are forced from callbacks to force checks like useUnconstrainedHeight to be correct, which is bad for performance, but also calculations that happen outside of the React lifecycle, like with the RowHeightUtils class, unexpectedly do not happen at all in some cases, leading to bugs like this. A well done custom hooks approach could solve both problems at once. I have a branch somewhere with EuiDataGridCell converted from a class based component to a functional one to enable this I think.

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

@kqualters-elastic kqualters-elastic requested a review from a team as a code owner December 19, 2024 03:57
@kqualters-elastic
Copy link
Contributor Author

If anyone is interested https://github.com/kqualters-elastic/eui/tree/row-height-debug has console.logs in the places where you can see the calculation fail, adding a height of 0 to the unconstrained height state, and then (incorrectly) incrementing the knownRowCount. For reproducing, happened immediately on my OSX laptop, on Windows desktop, I had to mess with the zoom level of the browser + the size of the window in order to trigger the bug.

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💔 Build Failed

Failed CI Steps

@kqualters-elastic
Copy link
Contributor Author

The failing test is https://github.com/elastic/eui/blob/main/packages/eui/src/components/datagrid/data_grid.spec.tsx#L66 and is probably a bad test IMO. The test is not measuring the unconstrained height from the useUnconstrainedHeight hook, but finalHeight from useFinalGridDimensions. That hook has a height state that would have to be destroyed, or set to 0 at some point, so that this ternary uses unconstrainedHeight

  const finalHeight = isFullScreen
    ? fullScreenHeight
    : height || unconstrainedHeight;

and I think the only reason it passes is because in one of the calculation loops, all heights are 0, similar to the bug in a way.

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.

Eui Datagrid height collapses to 0 when rows as of auto height
3 participants