[EuiDataGrid] Allow consuming applications to control various internal state via ref#5590
[EuiDataGrid] Allow consuming applications to control various internal state via ref#5590
ref#5590Conversation
…5499) * Set up types * Set up forwardRef * Add setFocusedCell API to returned grid ref obj * Add colIndex prop to cell actions - so that cell actions that trigger modals or flyouts can re-focus into the correct cell using the new ref API * Add documentation + example + props * Add changelog * [PR feedback] Types Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * [PR feedback] Clean up unit test * [Rebase] Tweak useImperativeHandle location - Moving it below fullscreen logic, as we're oging to expose setIsFullScreen as an API shortly Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
* Expose `setIsFullScreen` to ref API * Update documentation/examples
#5550) * [setup] Update testCustomHook to expose fn that allows accessing most recent state/value - without this callback, the initial returned hook values will be stale/not properly return most recent values - see next commit for example usage within useCellPopover * Set up cell popover context - set up initial open/location state, + open/close popover APIs returned to consumers - improve auto props documentation - remove EuiDataGridCellLocation in favor of specifying rowIndex and colIndex (it's less DRY but it's easier for devs to not have to look up EuiDataGridCellLocation from our docs) * Pass down popoverContext to cells as a prop - I'm not using context here because we're already using this.context for focus, and unfortunately class components can only initialize one context at time using `static contextType` (see https://reactjs.org/docs/context.html#classcontexttype) * Remove internal cell popoverIsOpen state - This should now be handled by the overarching context state, and the cell should simply react to it or update it (similar to how focusContext works) + add new var for hasCellButtons + add unit tests for isFocusedCell alongside isPopoverOpen (since both methods perform similar functions) * Update cell popovers to set the popover anchor & content - content is TODO, will likely be easier to compare when cleaning it up/moving it all at once * Refactor EuiDataGridCellPopover - It should no longer exist as a reusable component that belongs to every single cell, but instead as a single popover that exists at the top grid level and moves from cell to cell - I cleaned and split up the JSX for the popover (e.g. moving popover actions to data_grid_cell_buttons, where it feels like it belongs more) and think it's significantly more DRY now - note the entire `anchorContent` branch removed from EuiDataGridCell that is no longer necessary - Note that due to this change, we now have to mock EuiWrappingPopover in EuiDataGrid tests, as we see failures otherwise * [bugfix] Handle cells with open popover being scrolled out of view - this is the same behavior as in prod - causes weird DOM issues if we don't close the cell popover automatically * [bugfix] Workaround for popover DOM stuttering issues * [enhancement] Account for openCellPopover being called on cells out of view + write bonus Cypress test for useScroll's focus effect now that we have access to the imperative ref * Update documentation example + remove code snippet - it was starting to get redundant with the API bullet points, and is already available as tab if needed + fix control button widths * [PR feedback] Be more specific about useImperativeHandle dependencies + add a few explanatory comments * [PR feedback] Rename openCellLocation to cellLocation - to make it sound less like a verb/method * [PR feedback] Ignore edge case of `openCellPopover` being called on an `isExpandable: false` cell
…, off-page, or out of view cells (#5572) * Enable sorting + targeting row indices outside of the current page - to test handling the exposed APIs when dealing with sorted/paginated data * Switch data grid example to Typescript - to test type issues during consumer usage + @ts-ignore faker complaints * Fix cell expansion buttons on paginated pages not working correctly * Attempt to more clearly document `rowIndex`s that are actually `visibleRowIndex`s * [setup] Move imperative handler setup to its own util file - this will let us set up ref-specific helpers & add more comment context without bloating the main file * Add catch/check for cell locations that do not exist in the current grid * Add getVisibleRowIndex helper - Converts the `rowIndex` from the consumer to a `visibleRowIndex` that our internal state can use - Account for sorted grid by finding the inversed index of the `sortedRowMap` - To make this easier, I converted soredRowMap to an array (since it's already only uses numbers for both keys and values), since arrays have a handy .findIndex method - Handles automatically paginating the grid if the targeted cell is on a different page * Replace grid ref Jest tests with more complete Cypress tests * Update documentation with new behavior * [PR feedback] Rename fns to indicate multiple concerns - having a side effect in a getter feels bad, so change that to a `find` - rename use hook to indicate sorting and pagination concerns
|
@miukimiu I'm hoping for feedback on https://eui.elastic.co/pr_5590/#/tabular-content/data-grid-ref (copy, layout, etc.)! I've been working too long on this and I can't tell anymore if my docs are useful or not haha 🙈 |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5590/ |
elizabetdev
left a comment
There was a problem hiding this comment.
Thanks, @constancecchen! The PR LGTM! 🎉
Tested in Chrome, Safari, Edge, and Firefox.
To improve the docs, I opened a PR with some changes: cee-chen#5. Let me know if you agree with the changes.
|
Looks great to me, thanks Elizabet!! I'll cherry-pick your commit directly and push it up if that's cool ✨ |
- primarily for use within trailing/leading cells and custom actions - see 1609e45
|
@chandlerprall I remembered that our main datagrid example also needs to restore cell focus on modal+flyout close (#5477) and pushed up 2 commits that handles passing the |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5590/ |
chandlerprall
left a comment
There was a problem hiding this comment.
! I also tested the main datagrid.js example to verify flyout & modal both return focus.
This PR contains fixes for the following issues: #### # 1 Popover overlaps flyout - #139280 - #128235 #### # 2 Popover persists after clicking filter out - #115341 #### # 3 Popover persists after clicking a button outside of popover - #118844 ## Background Previously, a cell's popover remains open after clicking an action. In many cases we want the popover to close upon clicking on a cell action. EUI team addressed this by adding a `closeCellPopover` to a `ref` API. - elastic/eui#5590 In T-grid, there are 2 types of cell actions: - Default cell actions such as filter in, filter out, add to timeline and copy. `closeCellPopover` is not used. - Formatted fields that have more information in the form of flyouts (host name, user name, ip, etc.) `closeCellPopover` prop is passed but currently not working as expected. This PR contains fixes for: - Fixing `closeCellPopover` in T-grid body for formatted fields - fixes # 1 - Adding `closeCellPopover` props in default cell actions - fixes # 2 and # 3 ## # 1 - `closeCellPopover` in T-grid `dataGridRef.current?.closeCellPopover` was added and intended to close any open popovers when a cell action is clicked. However, because it is a mutable object, it is not being monitored in `columnsWithCellActions`. When the page is initially loaded, `dataGridRef.current` remain as null and it does not update until the page re-renders and `dataGridRef` becomes non-null. - After: popover closes properly https://user-images.githubusercontent.com/18648970/201202326-ec657f78-c425-46a6-9356-f6e9ef1ab798.mov ## # 2 & # 3 Add `closeCellPopover` to default cell actions - After: upon opening the expansion popover, clicking any options and the popover will disappear https://user-images.githubusercontent.com/18648970/201417542-063c514b-5474-4676-a747-a9401627c5e8.mov - After: upon opening the expansion popover, clicking any options outside and the popover will disappear https://user-images.githubusercontent.com/18648970/201417678-7cf0fefa-f4a7-4a70-9a10-76b248323639.mov Note for UX: although QA only flagged `filter out` and `add to timeline`, for consistency's sake, the expansion popover will disappear after clicking any of the cell actions, which includes `filter in` and `copy`.

Summary
List of initial APIs that our imperative
refis shipping with:setIsFullScreen(boolean)setFocusedCell({ rowIndex, colIndex})openCellPopover({ rowIndex, colIndex })closeCellPopover()Composite PRs:
refthat exposes focus/popover internal APIs #5499 (setFocusedCell)setIsFullScreento ref API #5531 (setIsFullScreen)openCellPopoverandcloseCellPopoverto ref APIs #5550 (openCellPopoverandcloseCellPopover)rowIndexfix to account for sorting and pagination)Screencaps
Checklist
Individual QA should have been tested/reviewed in prior feature branch PRs.