-
Notifications
You must be signed in to change notification settings - Fork 842
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] Add renderCustomGridBody
API
#6624
Conversation
…asses - base class has no virtualization dependencies and will be used by the custom renderer - The extended `RowHeightVirtualizationUtils` class will be used by the virtualized body renderer[Setup] Split up `RowHeightUtils` into virtualized/non-virtualized classes
Split up parent body into a basic ternary, and move tests accordingly Custom renderer logic will be in a separate commit (to hopefully make DRYing things out easier to follow)
- between custom & virtualized data grid bodies they each still need another curried _Cell wrapper on top of that to handle individual style or prop transmogs, but this DRYes out a significant amount of reused logic (and allows for clearer unit testing)
- the empty control columns array fallbacks were otherwise causing unnecessary rerenders in the custom renderer's memoized `visibleColumns`, which was causing cell popovers to break due to cells unmounting
- the current CSS takes for granted that the virtualization library is setting position+widths which is no longer always the case - our CSS should now provide sane fallbacks for custom rendering
548e99c
to
9f40db4
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_6624/ |
- forgot to run locally with new compiled CSS, which was causing a different outer vs scroll height computation
Preview documentation changes for this PR: https://eui.elastic.co/pr_6624/ |
Hey @weltenwort - just a quick ping on this PR. I was hoping to merge this in sometime either this week or the next (during EAH) - does that work with your current workload/timelines, or would you prefer to take a little longer? No worries if so, but some kind of ETA I can give folks would be super helpful, thanks! |
@cee-chen thanks for the reminder. I enjoyed reading it and didn't see any problem with the implementation. Not sure how much progress I'll make with my incremental loading POC this week, but independent of that I'm pretty sure this deserves a 👍 by tomorrow. |
Awesome, thank you! 🎉 If you have an ETA for your POC I'm totally cool with waiting for it, just want to make sure I have an approximate release date I can give folks. Either way though, if you run into issues, we can always add more fixes/needed features as needed, so I'm good with either merging sooner or merging later :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really great and I appreciate all the effort you've put into it to make it readable. 👏 I left a small question below.
<div className="euiDataGrid__customRenderBody"> | ||
{headerRow} | ||
{renderCustomGridBody!({ | ||
visibleColumns, | ||
visibleRowData: visibleRows, | ||
Cell: _Cell, | ||
})} | ||
{footerRow} | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows for completely custom rendering of the data rows, which is great. But it doesn't give access to the scrolling element itself, which would be interesting if the custom body implementation wants to attach event handlers (such as onScroll
), set scroll position (element.scrollTop = 0;
) or add styles (such as pointer-events: none
while scrolling). Would it be possible to structure this in a way so that is possible? I can think of two approaches:
- Introduce
customGridBodyProps
to the grid which are forwarded to the scrollingdiv
. - Leave rendering the scrolling
div
, header and footer up to the custom renderer by passing their factories or elements in the arguments.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great call! I'm leaning toward customGridBodyProps
which are passed as a spread to .euiDataGrid__customRenderBody
. Any objections?
I'm currently somewhat hesitant about leaving the header and footer entirely to the custom renderer. A lot of header functionality (e.g. sorting, etc) coming for free from EuiDataGrid is the appeal of using the the data grid in the first place. I think I'd prefer to keep it as-is currently but be open to tweaking it in the future if a compelling use case pops up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, just realized that I also needed to add explicit type support for passing a ref
to the div (for the element.scrollTop = 0
scenario you described): c1d280f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, props with the ref
should enable that use-case. 👍 It adds one more prop to the zoo, but on the other hand you retain control over the header and footer.
…derCustomGridBody` + improve docs slightly - note: I skipped adding documentation for `setCustomGridBodyProps` to snippets as this is technically an optional prop to use (unlike the others) - hopefully demo+prop table documentation are sufficient
Preview documentation changes for this PR: https://eui.elastic.co/pr_6624/ |
); | ||
|
||
// Allow consumers to pass custom props/attributes/listeners etc. to the wrapping div | ||
const [customGridBodyProps, setCustomGridBodyProps] = useState< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the disadvantage of delaying the propagation of the props by one render cycle. How about specifying these props when rendering the grid (similar to the virtualization options)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I phrased that ambiguously in my initial comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh apologies. I was hoping to keep the same paradigm/API that setCellProps
and setCellPopoverProps
has without adding Yet Another Top Level prop to EuiDataGrid. 😬 Is that one render cycle critical for your use case? If not, is the developer experience tradeoff worth the performance tradeoff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can follow the consistency argument and I can't point to any specific problem the setProps
in useEffect
approach would cause right now. 👍
That question was mainly driven by the anecdotal experience that such flashes of unstyled or uncontrolled content can cause layout shift and race conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can definitely re-evaluate this decision if you run into race conditions in the future! Thanks so much for your flexibility on this Felix - fingers crossed that this new API unblocks you and the Security team! 🎉
Preview documentation changes for this PR: https://eui.elastic.co/pr_6624/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely enables additional use-cases which need more control over the grid body. Thanks for investing all the effort.
// The custom row details is actually a trailing control column cell with | ||
// a hidden header. This is important for accessibility and markup reasons | ||
// @see https://fuschia-stretch.glitch.me/ for more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL, thank you 👍
## Summary eui@76.0.2 ⏩ eui@76.3.0 ## [`76.3.0`](https://github.com/elastic/eui/tree/v76.3.0) - Updated `EuiSkipLink`'s `fallbackDestination` prop to support an array of query selector strings ([#6646](elastic/eui#6646)) **Bug fixes** - Fixed `EuiFlyout` to preserve body scrollbar width on open ([#6645](elastic/eui#6645)) - Fixed `EuiImage`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([#6645](elastic/eui#6645)) - Fixed `EuiCodeBlock`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([#6645](elastic/eui#6645)) ## [`76.2.0`](https://github.com/elastic/eui/tree/v76.2.0) - Added new `renderCustomGridBody` escape hatch rendering prop to `EuiDataGrid` ([#6624](elastic/eui#6624)) **Bug fixes** - Fixed visual listbox focus ring bug on non-searchable `EuiSelectable`s ([#6637](elastic/eui#6637)) - Added a legacy `alert` alias for the `warning` `EuiIcon` type ([#6640](elastic/eui#6640)) - Fixed a type definition incorrectly coming from a dev dependency, which was causing issues for some consuming projects ([#6643](elastic/eui#6643)) ## [`76.1.0`](https://github.com/elastic/eui/tree/v76.1.0) - Added more detailed screen reader instructions to `EuiSelectable`, `EuiSuggest`, `EuiSelectableTemplateSitewide`, `EuiRange`, and `EuiDualRange`. ([#6589](elastic/eui#6589)) - Added new `placeholder` prop to `EuiSuperSelect` ([#6630](elastic/eui#6630)) - Added new `setCellPopoverProps` parameter callback to `EuiDataGrid`'s `renderCellPopover` prop ([#6632](elastic/eui#6632)) **Bug fixes** - Fixed an ARIA attribute in `EuiSelectableList` ([#6589](elastic/eui#6589)) - Fixed `EuiSelectable` to no longer show active selection state or respond to the Up/Down arrow keys when focus is inside the selectable container, but not on the searchbox or listbox. ([#6631](elastic/eui#6631)) --------- Co-authored-by: Tiago Costa <tiago.costa@elastic.co>
## Summary eui@76.0.2 ⏩ eui@76.3.0 ## [`76.3.0`](https://github.com/elastic/eui/tree/v76.3.0) - Updated `EuiSkipLink`'s `fallbackDestination` prop to support an array of query selector strings ([elastic#6646](elastic/eui#6646)) **Bug fixes** - Fixed `EuiFlyout` to preserve body scrollbar width on open ([elastic#6645](elastic/eui#6645)) - Fixed `EuiImage`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([elastic#6645](elastic/eui#6645)) - Fixed `EuiCodeBlock`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([elastic#6645](elastic/eui#6645)) ## [`76.2.0`](https://github.com/elastic/eui/tree/v76.2.0) - Added new `renderCustomGridBody` escape hatch rendering prop to `EuiDataGrid` ([elastic#6624](elastic/eui#6624)) **Bug fixes** - Fixed visual listbox focus ring bug on non-searchable `EuiSelectable`s ([elastic#6637](elastic/eui#6637)) - Added a legacy `alert` alias for the `warning` `EuiIcon` type ([elastic#6640](elastic/eui#6640)) - Fixed a type definition incorrectly coming from a dev dependency, which was causing issues for some consuming projects ([elastic#6643](elastic/eui#6643)) ## [`76.1.0`](https://github.com/elastic/eui/tree/v76.1.0) - Added more detailed screen reader instructions to `EuiSelectable`, `EuiSuggest`, `EuiSelectableTemplateSitewide`, `EuiRange`, and `EuiDualRange`. ([elastic#6589](elastic/eui#6589)) - Added new `placeholder` prop to `EuiSuperSelect` ([elastic#6630](elastic/eui#6630)) - Added new `setCellPopoverProps` parameter callback to `EuiDataGrid`'s `renderCellPopover` prop ([elastic#6632](elastic/eui#6632)) **Bug fixes** - Fixed an ARIA attribute in `EuiSelectableList` ([elastic#6589](elastic/eui#6589)) - Fixed `EuiSelectable` to no longer show active selection state or respond to the Up/Down arrow keys when focus is inside the selectable container, but not on the searchbox or listbox. ([elastic#6631](elastic/eui#6631)) --------- Co-authored-by: Tiago Costa <tiago.costa@elastic.co>
Summary
closes #6523 - please see the linked issue for rationale as to why this API was added, as well as other teams and use cases that might use it.
Code review: I seriously can't recommend following along by commit enough. I tried to make commits as granular and easy to read as possible, but unfortunately there's no getting around the fact that this is a lot of code that needs to be refactored and tested.
QA
General checklist
(using@default
if default values are missing) and playground toggles- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for breaking changes and labeled appropriately- [ ] Updated the Figma library counterpart