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

[EuiDataGrid] Replace popoverContents API with renderCellPopover #5640

Merged
merged 22 commits into from
Feb 16, 2022

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Feb 15, 2022

Summary

closes #5310

This PR deprecates the old popoverContents API:

popoverContents={{
  numeric: ({ children, cellContentsElement }) => {
    const stringContents = cellContentsElement.textContent;
    // extract the groups-of-three digits that are right-aligned
    return stringContents.replace(/((\\d{3})+)$/, match =>
      // then replace each group of xyz digits with ,xyz
      match.replace(/(\\d{3})/g, ',$1')
    );
  },
}}

With a new (optional) renderCellPopover API, that overrides our default popover rendering and allows users to completely customize cell popovers:

// 1:1 conversion of above API example
renderCellPopover={({ children, cellContentsElement }) => {
  const stringContents = cellContentsElement.textContent;
  return stringContents.replace(/((\\d{3})+)$/, match =>
    match.replace(/(\\d{3})/g, ',$1')
  );
}}

// Example that replicates our default popover
renderCellPopover={({ children, cellActions }) => (
  <>
    <EuiText>{children}</EuiText>
    {cellActions}
  </>
)}

// Example that removes the EuiText wrapper, but keeps the default cell actions
renderCellPopover={({ children, cellActions }) => (
  <>
    <div className="custom">{children}</div>
    {cellActions}
  </>
)}

// Example that sets completely custom popover content
renderCellPopover={() => (
  <>
    <EuiPopoverTitle>Hello world</EuiPopoverTitle>
    <p>Lorem ipsum dolor sit amet</p>
    <EuiPopoverFooter>
      <EuiButtonEmpty size="xs">Custom action</EuiButtonEmpty>
    </EuiPopoverFooter>
  </>
)}

// Example that conditionally renders certain popovers as custom, but others as default popovers
renderCellPopover={(props) => {
  const { schema, DefaultCellPopover } = props;

  if (schema === 'custom') {
    return <MyCustomPopover {...props} />;
  } else {
    return <DefaultCellPopover {...props} />;
  }
}}

Notes

  • In a normal EuiDataGrid without a renderCellPopover prop, nothing should have regressed or changed from previous popover rendering/behavior.
  • ⚠️ I strongly recommend following along by commit.
    • Most of the added lines are documentation and Cypress tests.
    • The popover logic itself is not all that different before line-diff-wise, it's just a slightly different architecture.

Screencap

screencap

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
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

- which replaces `popoverContents` and shares some common props with `renderCellValue`
+ improve prop documentation for renderCellValue, renderFooterCellValue
+ move to data_grid_cell_popover instead of popover_utils
- the popover needs this to conditionally change formatting based on schema, and renderCellValue seems like it could use this info as well
- which will allow custom renderers to pass back the default renderer if they only want custom rendering for certain popovers
- likely created by recent Cypress reference/ts PRs
+ remove props unrelated to schema

+ schema.js misc cleanup:
- Remove unnecessary pagination state/props - there's only 5 items
- Remove unnecessary conditional JSON
- Make Star Wars vs Star Trek alternating, so sorting does more
- Fix aria-label
- (lint) fragment, export default
- remove popoverContents, add renderCellPopover
- improve documentation on rendercellValue
- Fix several incorrect instances of `optional` vs `required` notation
- move inMemory down to sorting/pagination instead of being at the top, since it's an optional prop
- misc indentation/lint fixes
@cee-chen cee-chen force-pushed the datagrid/render-cell-popover branch from c19294a to 7b52c07 Compare February 15, 2022 21:52
@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple thoughts, but I don't see any functional issues. This turned out really clean and straight-forward to use

src/components/datagrid/data_grid_types.ts Outdated Show resolved Hide resolved
src/components/datagrid/body/data_grid_cell.tsx Outdated Show resolved Hide resolved
@cee-chen cee-chen force-pushed the datagrid/render-cell-popover branch from 3582b4a to 72501ed Compare February 16, 2022 21:25
@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM; pulled & tested locally and clicked around in the pr preview docs quite a bit too

@cee-chen
Copy link
Member Author

Woohoo!!!

@cee-chen cee-chen merged commit 93b70f9 into elastic:main Feb 16, 2022
@cee-chen cee-chen deleted the datagrid/render-cell-popover branch February 16, 2022 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiDataGrid] Expansion popover needs more customization
3 participants