[EuiDataGrid] Set up `ref` that exposes focus/popover internal APIs #5499

cee-chen · 2021-12-21T18:41:07Z

Summary

setFocusedCell.mp4

https://eui.elastic.co/pr_5499/#/tabular-content/data-grid-ref-methods

What this PR does:

Adds a forwardRef and useImperativeHandle to EuiDataGrid
Exposes the setFocusedCell API only

What this PR does not do:

Add a closeCellPopover or openCellPopover API (that will be the next PR, since it requires a new context for EuiDataGridCells to communicate with EuiDataGrid and a decent amount of refactoring)
Re-focusing to the cell from the cell popover actions does not currently work. This is because the popover is trapping focus within the popover. This will begin working once we add the closeCellPopover API.
Attempting to setFocusedCell to a location that is currently outside the virtualization window (i.e. not rendered) currently does not work, but I have a fix for this that addresses open focus/scroll bugs that will be PR'ed against main.

⚠️ Notes:

This PR is against a feature branch for [EuiDataGrid] Expansion popover needs more customization #5310 (since it will likely take multiple PRs to accomplish)
I recommend turning off whitespace changes while viewing the diff, since the wrapping forwardRef created a ton of indentation in data_grid.tsx

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
Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately (This PR is going into a feature branch, and additionally is only adding new APIs)
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

src/components/datagrid/data_grid_types.ts

src-docs/src/views/datagrid/datagrid_ref_example.js

cee-chen · 2021-12-21T18:56:39Z

src-docs/src/views/datagrid/ref.js

+  // Manual cell focus
+  const [rowIndexFocus, setRowIndexFocus] = useState(0);
+  const [colIndexFocus, setColIndexFocus] = useState(0);
+
+  return (
+    <>
+      <EuiFlexGroup alignItems="flexEnd" gutterSize="s" style={{ width: 500 }}>
+        <EuiFlexItem>
+          <EuiFormRow label="Row index">
+            <EuiFieldNumber
+              min={0}
+              max={25}
+              value={rowIndexFocus}
+              onChange={(e) => setRowIndexFocus(e.target.value)}


Is there any way to separate out the input JSX/state (which is more of a demo) from the actual modal example/code, so that the final Demo JS that people see doesn't include these controlling flex group/inputs? I think Caroline did something like that in this PR but I'm actually not sure how now that I look at it again 🙈

Alternatively if y'all don't think it's worth it or think this demo is fine as-is, I can scrap that idea

The workaround Caroline used in the date picker width example is to import the full example but only the source for the piece desired in the code tab,

import SuperDatePickerWidthExample from './super_date_picker_width_example'; const superDatePickerWidthSource = require('!!raw-loader!./super_date_picker_width'); ... source: [ { type: GuideSectionTypes.TSX, code: superDatePickerWidthSource, // what is wanted in the code tab }, ], demo: <SuperDatePickerWidthExample />, // full interactive example

I'm good with doing the same in this example, but we should revisit with the team to see what folks think of this pattern and if there's a way to standardize it.

I love the idea of standardizing it or adding a DRY helper for this somehow! I'll probably leave this current PR as-is, but I'll add a new discussion item for our next sync for this

cee-chen · 2021-12-21T18:59:14Z

src/components/datagrid/data_grid.tsx

+export const EuiDataGrid = forwardRef<EuiDataGridRefProps, EuiDataGridProps>(
+  (props, ref) => {
+    const {


This is mostly just whitespace/indenting changes that can be turned off

kibanamachine · 2021-12-21T19:11:57Z

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

kibanamachine · 2021-12-21T20:34:42Z

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

chandlerprall

Re-focusing to the cell from the cell popover actions does not currently work. This is because the popover is trapping focus within the popover. This will begin working once we add the closeCellPopover API.

What do you think about making setFocusedCell close any open popovers?

src/components/datagrid/data_grid.test.tsx

src-docs/src/views/datagrid/datagrid_ref_example.js

src/components/datagrid/data_grid_types.ts

cee-chen · 2022-01-04T19:00:03Z

What do you think about making setFocusedCell close any open popovers?

I'm pretty medium on this because I think it's both potentially unexpected and sets a risky precedent for exposing internal APIs with side effects. When I call setFocusedCell as an exposed API, I would expect it to do exactly what the internal method does.

It might also be worth taking a look at the next upcoming PR which will add the closePopover/openPopover APIs. You can see in the documentation example that 1 extra line really isn't all that onerous, and I actually ended up adding closePopover() in the open modal action, not in the close modal action which restores cell focus. We allow for that kind of extra flexibility by not conflating the two actions together.

kibanamachine · 2022-01-04T19:27:46Z

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

kibanamachine · 2022-01-05T19:22:42Z

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

- so that cell actions that trigger modals or flyouts can re-focus into the correct cell using the new ref API

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

kibanamachine · 2022-01-05T22:10:13Z

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

- Moving it below fullscreen logic, as we're oging to expose setIsFullScreen as an API shortly

kibanamachine · 2022-01-06T00:14:22Z

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

chandlerprall

LGTM, ref example works without issue and I notice any regressions elsewhere. Feel free to rework the demo code if you'd like before merging, or leaving as-is.

@ts-ignore

…l state via `ref` (#5590) * [EuiDataGrid] Set up `ref` that exposes focus/popover internal APIs (#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> * [EuiDataGrid] Add `setIsFullScreen` to ref API (#5531) * Expose `setIsFullScreen` to ref API * Update documentation/examples * [EuiDataGrid] Add `openCellPopover` and `closeCellPopover` to ref APIs (#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 * [EuiDataGrid] Handle exposed ref APIs potentially pointing to invalid, 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 * Improve changelog * Data grid ref methods docs review * Fix colIndex to be available in renderCellValue as well as cell actions - primarily for use within trailing/leading cells and custom actions - see 1609e45 * Fix main datagrid example to restore cell focus on modal/flyout close * [a11y] Fix missing header cell text in main datagrid example Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: Elizabet Oliveira <elizabet.oliveira@elastic.co>

cee-chen commented Dec 21, 2021

View reviewed changes

src/components/datagrid/data_grid_types.ts Outdated Show resolved Hide resolved

cee-chen requested review from chandlerprall and thompsongl December 21, 2021 18:53

cee-chen commented Dec 21, 2021

View reviewed changes

src-docs/src/views/datagrid/datagrid_ref_example.js Show resolved Hide resolved

cee-chen commented Dec 21, 2021

View reviewed changes

cee-chen force-pushed the datagrid/5310/ref branch from efc9fff to bd09f38 Compare December 21, 2021 20:04

chandlerprall reviewed Dec 30, 2021

View reviewed changes

src/components/datagrid/data_grid.test.tsx Outdated Show resolved Hide resolved

src-docs/src/views/datagrid/datagrid_ref_example.js Show resolved Hide resolved

src/components/datagrid/data_grid_types.ts Outdated Show resolved Hide resolved

cee-chen and others added 8 commits January 5, 2022 13:25

Set up types

43e1c69

Set up forwardRef

681f9d9

Add setFocusedCell API to returned grid ref obj

14401ef

Add colIndex prop to cell actions

1609e45

- 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

b51a614

Add changelog

3b7c4e2

[PR feedback] Types

941f0fa

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

[PR feedback] Clean up unit test

cd2b235

cee-chen force-pushed the datagrid/5310/ref branch from 7670c4e to cd2b235 Compare January 5, 2022 21:36

[Rebase] Tweak useImperativeHandle location

b6ac09f

- Moving it below fullscreen logic, as we're oging to expose setIsFullScreen as an API shortly

chandlerprall approved these changes Jan 11, 2022

View reviewed changes

cee-chen merged commit 59461c7 into elastic:feat/datagrid/5310 Jan 11, 2022

cee-chen deleted the datagrid/5310/ref branch January 11, 2022 21:30

cee-chen mentioned this pull request Feb 1, 2022

[EuiDataGrid] Allow consuming applications to control various internal state via ref #5590

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Set up `ref` that exposes focus/popover internal APIs #5499

[EuiDataGrid] Set up `ref` that exposes focus/popover internal APIs #5499

cee-chen commented Dec 21, 2021 •

edited

Loading

cee-chen Dec 21, 2021 •

edited

Loading

chandlerprall Jan 11, 2022

chandlerprall Jan 11, 2022

cee-chen Jan 11, 2022

cee-chen Dec 21, 2021

kibanamachine commented Dec 21, 2021

kibanamachine commented Dec 21, 2021

chandlerprall left a comment

cee-chen commented Jan 4, 2022

kibanamachine commented Jan 4, 2022

kibanamachine commented Jan 5, 2022

kibanamachine commented Jan 5, 2022

kibanamachine commented Jan 6, 2022

chandlerprall left a comment

[EuiDataGrid] Set up ref that exposes focus/popover internal APIs #5499

[EuiDataGrid] Set up ref that exposes focus/popover internal APIs #5499

Conversation

cee-chen commented Dec 21, 2021 • edited Loading

Summary

Checklist

cee-chen Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

chandlerprall Jan 11, 2022

Choose a reason for hiding this comment

chandlerprall Jan 11, 2022

Choose a reason for hiding this comment

cee-chen Jan 11, 2022

Choose a reason for hiding this comment

cee-chen Dec 21, 2021

Choose a reason for hiding this comment

kibanamachine commented Dec 21, 2021

kibanamachine commented Dec 21, 2021

chandlerprall left a comment

Choose a reason for hiding this comment

cee-chen commented Jan 4, 2022

kibanamachine commented Jan 4, 2022

kibanamachine commented Jan 5, 2022

kibanamachine commented Jan 5, 2022

kibanamachine commented Jan 6, 2022

chandlerprall left a comment

Choose a reason for hiding this comment

[EuiDataGrid] Set up `ref` that exposes focus/popover internal APIs #5499

[EuiDataGrid] Set up `ref` that exposes focus/popover internal APIs #5499

cee-chen commented Dec 21, 2021 •

edited

Loading

cee-chen Dec 21, 2021 •

edited

Loading