[EuiDataGrid] Major reorganization of data_grid.tsx and data_grid_body.tsx
#5506
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Rename data_grid_inmemory_renderer to utils/in_memory (other util files will also be added here shortly)
+ move DataGridFocusContext to its said file to keep concerns together
- this was already going to happen in a separate PR/bugfix and this commit is going to get rebased out, I'm just adding it here now to make types less annoying
- NB: it still gets stored via focusContext
- since virtualization, contentRef is not in charge of scrolling, the virtualized grid is, and manages sticky headers via `top` styles, so AFAICT this use effect does nothing
+ removed reference (here and in previous commit) - I don't see a need for it as setting a new focused cell will cause a rerender anyway - adding the comment block status as there will be more focus workarounds for (e.g.) keyboard navigation coming soon
…col counts - Remove passing entire props to createKeyDownHandler, be more deliberate about args being passed - move computeVisibleRows to its own file - calculate visible row & col count at top datagrid level instead of in the keydown handler (this will be passed down to the body to DRY out even more logic in the next commit)
- it's almost the exact same calculations being done in computeVisibleRows - no need to repeat it twice + refactor out unnecsesary for loop/array over subtraction
- Move all width related logic to EuiDataGridBody
- this seems like a better place to move grid height/weight logic to (since react-window's Grid component is doing the majority of the calculations)
- we now also only have to pass down 1 column prop instead of 3 to the body
- remove `defaultColumnWidth` check - this was added pre-virtualization and doesn't appear to be necessary any longer (functions fine without it/all tests pass)
- move column width related utils to their own util/col_widths file
- rename `getWidth` -> `getColumnWidth` & move callback to use `useColumnWidths`
- refactor static 100 width num to a constant
- move `useVirtualizeContainerWidth` to separate grid_height_width util (which useConstrainedHeight` logic will also shortly move to)
- pass outerGridRef directly in instead of having to query for the virtualized container
+ remove now unnecessary `VIRTUALIZED_CONTAINER_CLASS` constant
…ght to util file - should mostly have been moved as-is without significant logic change
- Rename `row_height_utils` to `utils/row_heights`, and add extra hooks/methods used in the grid body (w/ a few extra comments) - Move rowHeightUtils instantation from top level data grid to body - it isn't needed that high up, and in general I'm trying to move all height/width dimension calculations to the body where it can more directly interface with react-window's Grid - rename `defaultHeight`->`defaultRowHeight` for more clarity
- Move `data_grid_schema` to new `utils` folder (but keep naming, due to i18n tokens) - move `definedColumnSchema` and `detectedSchema` to internal schema file, since it isn't used elsewhere - add extra comment headings for organization to file - diffs are mostly just changing import locations (+ fix src-docs to point at shared _types file)
…l file + move context to new util file, and save both map & getCorrectRowIndex to context for easy sharing between descendants (without having to pass as a prop) + rename `rowMap` to more specific `sortedRowMap` - remove now-unnecessary inMemory passed props + refactor repeated correct row index code in Cell component to use getCorrectRowIndex
…l file - for better organization / separation of concerns / unit testing
- this matches other data (e.g. gridStyles, schema) that combines dev props with grid defaults, and is a cleaner pass-through
- Remove unnecessary `gridWidth` state - the only thing using it is the toolbar, which should respond accordingly to prop changes in any case on resize - rename vars more succinctly via destructuring, + update body to more clearly reflect that we're only observing the grid width - move comment to more applicable file
- We're already falling back to an empty array in the top-level datagrid, so there's no need to do so in the body as well - I'm not really sure what the need for emptyArrayDefault is vs. just using an array. I see this in git history as a perf optimization but I'm a little suspicious as to how big of an impact that could have over, e.g. memoizing displayValues - speaking of, memoized displayValues while I'm here
+ Remove unnecessary `props.` (props are already destructured) - No logic should have changed, just location
- the order is a little awkward here due to dependencies, but I still think it's cleaner/slightly easier to read and lays out potential repeats in a more easy to spot way
- It's easier to store these where they're being used / near their concerns, and in this case the last `DataGridWrapperRowsContext` is only used within data_grid_body
- The extra div/CSS appears to be unnecessary, and the mutation observer makes more sense in the top-level grid now that we're managing most focus behavior there
- it wasn't super clear to me what those divs were effectively doing, and from my brief testing our current virtualized grid works fine without it - keyboard navigation also appears to work without issue/with the onKeyboardDown handler moved
- this will allow us to pass gridRef to more than just EuiDataGridBody - for instance, pagination & focus will want to use `gridRef.scrollIntoView`
- putting this inside `onChangePage` feels better/has less potential for side effects or unnecessarily calling scrollToItem on mount - hopefully easier to write E2E tests for all paginated-related effects at once + moved data_grid_pagination into utils/ dir for consistency (keeping name the same due to i18n tokens)
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5506/ |
|
😮 Wow, thank you for this! Went through each commit & the changes make sense. Haven't done any significant testing yet. How would you like to coordinate these changes with other datagrid development? |
I'm holding off on a few more bugfixes until this refactor is in (as it generally makes writing code and tests easier). I'd ideally like to get it in sooner rather than later, but if necessary #5310 can likely get merged first. I'll handle all merge conflicts in my branches as well. Does that sound OK or do you have any strong thoughts there? |
|
I'm good with that plan, just curious which PR I should focus on reviewing. I'll start checking this one for regressions. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5506/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5506/ |
chandlerprall
suggested changes
Jan 5, 2022
- Focus context isn't available in data_grid.tsx, so the hook doesn't work there - it needs to be instantiated in the body
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5506/ |
chandlerprall
approved these changes
Jan 5, 2022
|
Thanks a million Chandler!! Merging, will keep a close eye out for issues or bugs (hopefully) before the next release |
cee-chen
pushed a commit
that referenced
this pull request
Jan 5, 2022
…body.tsx` (#5506) * [inMemory] Move useInMemoryValues to memory-specific util file - Rename data_grid_inmemory_renderer to utils/in_memory (other util files will also be added here shortly) * [focus] Move focus utils to separate util file + move DataGridFocusContext to its said file to keep concerns together * [focus] Add focusedCell to focus context - this was already going to happen in a separate PR/bugfix and this commit is going to get rebased out, I'm just adding it here now to make types less annoying * [focus] Move onFocusUpdate and focus cells map to useFocus - NB: it still gets stored via focusContext * [focus] Remove unnecessary scrollTop workaround - since virtualization, contentRef is not in charge of scrolling, the virtualized grid is, and manages sticky headers via `top` styles, so AFAICT this use effect does nothing * [focus/header] Move headerIsInteractive logic to its own file * [focus] Move header setFocusedCell workaround to its own useEffect + removed reference (here and in previous commit) - I don't see a need for it as setting a new focused cell will cause a rerender anyway - adding the comment block status as there will be more focus workarounds for (e.g.) keyboard navigation coming soon * [focus + row/col count] Clean up createKeyDownHandler, DRY out row & col counts - Remove passing entire props to createKeyDownHandler, be more deliberate about args being passed - move computeVisibleRows to its own file - calculate visible row & col count at top datagrid level instead of in the keydown handler (this will be passed down to the body to DRY out even more logic in the next commit) * [row/col count] DRY out row/col count logic repeated in EuiDataGridBody - it's almost the exact same calculations being done in computeVisibleRows - no need to repeat it twice + refactor out unnecsesary for loop/array over subtraction * [widths] Refactor width-related fns to separate util files - Move all width related logic to EuiDataGridBody - this seems like a better place to move grid height/weight logic to (since react-window's Grid component is doing the majority of the calculations) - we now also only have to pass down 1 column prop instead of 3 to the body - remove `defaultColumnWidth` check - this was added pre-virtualization and doesn't appear to be necessary any longer (functions fine without it/all tests pass) - move column width related utils to their own util/col_widths file - rename `getWidth` -> `getColumnWidth` & move callback to use `useColumnWidths` - refactor static 100 width num to a constant - move `useVirtualizeContainerWidth` to separate grid_height_width util (which useConstrainedHeight` logic will also shortly move to) - pass outerGridRef directly in instead of having to query for the virtualized container + remove now unnecessary `VIRTUALIZED_CONTAINER_CLASS` constant * [width/height] Move final grid width/height logic + useConstrainedHeight to util file - should mostly have been moved as-is without significant logic change * [row heights] Refactor more row heights logic to utils - Rename `row_height_utils` to `utils/row_heights`, and add extra hooks/methods used in the grid body (w/ a few extra comments) - Move rowHeightUtils instantation from top level data grid to body - it isn't needed that high up, and in general I'm trying to move all height/width dimension calculations to the body where it can more directly interface with react-window's Grid - rename `defaultHeight`->`defaultRowHeight` for more clarity * [schema] Move slightly more schema logic to separate schema file - Move `data_grid_schema` to new `utils` folder (but keep naming, due to i18n tokens) - move `definedColumnSchema` and `detectedSchema` to internal schema file, since it isn't used elsewhere - add extra comment headings for organization to file - diffs are mostly just changing import locations (+ fix src-docs to point at shared _types file) * [sorting] Move `rowMap` and `getCorrectRowIndex` logic to sorting util file + move context to new util file, and save both map & getCorrectRowIndex to context for easy sharing between descendants (without having to pass as a prop) + rename `rowMap` to more specific `sortedRowMap` - remove now-unnecessary inMemory passed props + refactor repeated correct row index code in Cell component to use getCorrectRowIndex * [focus] Move `preventTabbing` and `getParentCellContent` to focus util file - for better organization / separation of concerns / unit testing * [body] Move popover content merging from body to top-level grid - this matches other data (e.g. gridStyles, schema) that combines dev props with grid defaults, and is a cleaner pass-through * [grid] Clean up width/height observers - Remove unnecessary `gridWidth` state - the only thing using it is the toolbar, which should respond accordingly to prop changes in any case on resize - rename vars more succinctly via destructuring, + update body to more clearly reflect that we're only observing the grid width - move comment to more applicable file * [misc] Clean up control column fallbacks - We're already falling back to an empty array in the top-level datagrid, so there's no need to do so in the body as well - I'm not really sure what the need for emptyArrayDefault is vs. just using an array. I see this in git history as a perf optimization but I'm a little suspicious as to how big of an impact that could have over, e.g. memoizing displayValues - speaking of, memoized displayValues while I'm here * [grid] Organize code by concern & w/ comment headings + Remove unnecessary `props.` (props are already destructured) - No logic should have changed, just location * [body] Organize code by concern w/ comment headings - the order is a little awkward here due to dependencies, but I still think it's cleaner/slightly easier to read and lays out potential repeats in a more easy to spot way * [misc] Remove data_grid_context file - It's easier to store these where they're being used / near their concerns, and in this case the last `DataGridWrapperRowsContext` is only used within data_grid_body * [dom cleanup] Combine `dataGridWrapper` and `euiDataGridBody` divs - The extra div/CSS appears to be unnecessary, and the mutation observer makes more sense in the top-level grid now that we're managing most focus behavior there * [dom cleanup] Remove verticalScroll and overflow div wrappers - it wasn't super clear to me what those divs were effectively doing, and from my brief testing our current virtualized grid works fine without it - keyboard navigation also appears to work without issue/with the onKeyboardDown handler moved * [gridRef setup] Store gridRef at top-level data_grid - this will allow us to pass gridRef to more than just EuiDataGridBody - for instance, pagination & focus will want to use `gridRef.scrollIntoView` * [pagination] Move scroll fix into pagination concern - putting this inside `onChangePage` feels better/has less potential for side effects or unnecessarily calling scrollToItem on mount - hopefully easier to write E2E tests for all paginated-related effects at once + moved data_grid_pagination into utils/ dir for consistency (keeping name the same due to i18n tokens) * [toolbar/misc] DRY out IS_JEST_ENVIRONMENT * [PR feedback] Add breaking changelog * [PR feedback] Fix header focus workaround - Focus context isn't available in data_grid.tsx, so the hook doesn't work there - it needs to be instantiated in the body
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
👋 Here's my Christmas gift for @chandlerprall - a supermassive PR that will take a ton of time to review!
I've mentioned a couple of times now that the main
data_gridanddata_grid_bodyfiles feel hard to grok because there's a ton of disjointed/disconnected code spanning a ton of various different functionality, fixes, workarounds, etc. This PR should hopefully get us in the right direction of organizing code into modular utility files organized by concern (e.g., focus, heights, widths, etc.).What changed?
EuiDataGridandEuiDataGridBodynow lives with a newutils/folder inside its own conceptually-related utility file:CellandInnerElementinEuiDataGridBody, which I left alone - I thinkInnerElementmakes sense to keep but am thinking of movingCellinto its own separate file in thebody/dir in a later PR)data_grid.tsxwent from 925 lines to 472data_grid_body.tsxwent from 815 lines to 477What didn't change?
Functionality, nothing should have broken from this PR (fingers crossed - we'll likely need to do a lot of QA testing as well as performance testing).
I'm hoping to be able to write more unit tests and Cypress tests (since the per-file concerns makes it conceptually easier to write per-concern E2E tests) as a result of this cleanup.
Checklist
- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examplesAdded or updated jest and cypress tests(NB: Will add more unit tests in follow-up PRs)