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] Introduce renderCustomToolbar, additionalDisplaySettings and allowResetButton customization options #7190

Merged
merged 25 commits into from
Sep 25, 2023

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Sep 14, 2023

The initial work was done in #7148 by @timductive and @kertal

Summary

This PR allows more customization options for the grid toolbar.

  1. Full customization for the toolbar is now possible via the new renderCustomToolbar prop on EuiDataGrid by returning a completely custom react element:

https://github.com/jughosta/eui/blob/render-custom-toolbar/src/components/datagrid/data_grid_types.ts#L50

renderCustomToolbar={() => {
    return <div>Totally custom toolbar</div>;
  }}
  1. It's now possible to render additional elements inside the display settings popover and also hide the reset button

https://github.com/jughosta/eui/blob/render-custom-toolbar/src/components/datagrid/data_grid_types.ts#L872-L879

toolbarVisibility={{
  showDisplaySelector: {
    allowResetButton: false,
    additionalDisplaySettings: <div>extra settings</div>,
  },
}}

An example can be found under /tabular-content/data-grid-toolbar#custom-toolbar-layout

Screenshot 2023-09-20 at 13 50 07

Regarding a new design for the default toolbar buttons, I think we could address it in a separate PR.

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • Updated the Figma library counterpart

@jughosta
Copy link
Contributor Author

jughosta commented Sep 14, 2023

Hi @cee-chen @kertal @timductive,

I am continuing the work which was started in #7148 but decided to create a new PR as it would be easier for me to followup on changes.

Would be great to get an early feedback! I still need to add more tests.

@cee-chen
Copy link
Contributor

Hey @jughosta, thank you for this PR! As a heads up I was a little swamped today and ended up not having time to review. I'm planning on making it a priority for my Monday to review this PR.

@cee-chen cee-chen self-requested a review September 15, 2023 01:01
typeof showDisplaySelector === 'boolean'
? null
: showDisplaySelector?.additionalDisplaySettings ?? null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed exposing density and rowHeight controls as I am not planning to directly use them in Discover. Having additionalDisplaySettings looks cleaner to me than reimplementing the whole popover on a consumer side.

Copy link
Contributor

Choose a reason for hiding this comment

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

++ agreed, good call on this Julia!

Comment on lines 53 to 59
renderCustomToolbar?: (props: EuiDataGridCustomToolbarOptions) =>
| {
// to replace only controls inside the toolbar, use `left` and `right`
left?: ReactNode;
right?: ReactNode;
}
| ReactElement; // to replace the whole toolbar
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little marginal on the left/right options - if someone's rendering a custom toolbar, I expect them to take it over fully and for rendering responsibility to be fully on the consumer rather than remaining partially on EUI. I'd rather not have an in-between state.

@jughosta Do you mind diving a bit into the use-case you see for left/right replacement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cee-chen I am going to work on Discover side for it today and will provide more details later.

Copy link
Contributor

@cee-chen cee-chen Sep 19, 2023

Choose a reason for hiding this comment

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

👍 LMK if I can help at all! If possible, I would prefer consumers to do something like this:

({ hasRoomForGridControls }) => (
  <EuiFlexGroup responsive={false} gutterSize="s" justifyContent="spaceBetween">
    <EuiFlexItem grow={false}>
      {/* Left side controls, possibly behind a `hasRoomForGridControls` conditional */}
    </EuiFlexItem>
    <EuiFlexItem grow={false}>
      {/* Right side controls */}
    </EuiFlexItem>
  </EuiFlexGroup>
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cee-chen Thanks! I applied your suggestion 84eb5fd

You are right, it's relatively simple to recreate the layout which would be similar to the Eui default one. Changed this in the demo code too.

@cee-chen
Copy link
Contributor

cee-chen commented Sep 19, 2023

I think those are most of my higher-level comments Julia! Apologies for the mix of architectural comments and nits, the primary 2 blocking comments I have are around the left/right object API and the extra non-Control props being passed back via the render callback.

Quick FYI, I'll skip reviewing tests and documentation for now until we've settled on a more finalized API.

@jughosta jughosta changed the title [EuiDataGrid] Introduce renderCustomToolbar and additionalDisplaySettings customization options [EuiDataGrid] Introduce renderCustomToolbar, additionalDisplaySettings and allowResetButton customization options Sep 20, 2023
@jughosta jughosta marked this pull request as ready for review September 22, 2023 19:34
@jughosta jughosta requested a review from a team as a code owner September 22, 2023 19:34
- Remove now-unnecessary `closePopover()` call

- Remove separate reset button test that can be combined with the first test
@@ -135,6 +135,36 @@ describe('EuiDataGridToolbar', () => {
</div>
`);
});

it('renders custom content if renderCustomToolbar is defined', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

These are seriously great tests. Thank you for them!!

- more docs copy

- rename file with prop name

- promote caching the various custom elements by pulling them out to top-level `const`s

- remove props/extra demo details that aren't relevant to the toolbar
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

@kertal @timductive @jughosta Thank you for the seriously amazing PR!! :) Very in awe of your test writing skills and a huge 👏 for the clean-reading code.

I sincerely apologize for taking so long to get to back to this PR, last week was super busy. I had some very minor cleanup feedback and a docs related pass that I opted to push up directly to your branch to save y'all time and so we can get this work in for next week's release. LMK if you have any issues with the changes, otherwise I'll merge this in on Monday.

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen merged commit 7767b10 into elastic:main Sep 25, 2023
1 check passed
@cee-chen cee-chen self-assigned this Sep 25, 2023
@jughosta jughosta deleted the render-custom-toolbar branch September 26, 2023 12:26
jbudz pushed a commit to elastic/kibana that referenced this pull request Sep 27, 2023
`v88.3.0`⏩`v88.5.0`

closes #151514

---

## [`88.5.0`](https://github.com/elastic/eui/tree/v88.5.0)

- Updated `EuiCallOut` with a new `onDismiss` prop
([#7156](elastic/eui#7156))
- Added a new `renderCustomToolbar` prop to `EuiDataGrid`, which allows
custom rendering of the toolbar.
([#7190](elastic/eui#7190))
- Added a new `allowResetButton` prop to
`toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows
hiding the "Reset to default" button from the display settings popover.
([#7190](elastic/eui#7190))
- Added a new `additionalDisplaySettings` prop to
`toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows
rendering extra settings inside the display settings popover.
([#7190](elastic/eui#7190))
- Updated `EuiDataGrid`'s toolbar display settings button icon
([#7190](elastic/eui#7190))
- Updated `EuiTextTruncate` with significantly improved iteration
performance. Removed `measurementRenderAPI` prop, as `EuiTextTruncation`
now only uses more performant canvas render API
([#7210](elastic/eui#7210))
- Updated `EuiPopover` with a new configurable `repositionToCrossAxis`
prop ([#7211](elastic/eui#7211))
- Updated `EuiDatePicker` to support `compressed` input styling
([#7218](elastic/eui#7218))
- Added `gradient` and `palette` icon glyphs.
([#7220](elastic/eui#7220))

**Bug fixes**

- Fixed `EuiPopover`'s missing animations on popover close
([#7211](elastic/eui#7211))
- Fixed `EuiInputPopover` anchoring to the wrong side and missing
shadows on smaller screens
([#7211](elastic/eui#7211))
- Fixed `EuiSuperDatePicker` icon spacing on the quick select button
([#7217](elastic/eui#7217))
- Fixed a missing type in `EuiMarkdownEditor`'s default processing
plugins ([#7221](elastic/eui#7221))


## [`88.4.1`](https://github.com/elastic/eui/tree/v88.4.1)

**Bug fixes**

- Fixed missing `className`s on `EuiTextTruncate`
([#7212](elastic/eui#7212))
- Fixed `title`s on `EuiComboBox` dropdown options to always be present
([#7212](elastic/eui#7212))
- Fixed `EuiComboBox` truncation issues when search is an empty space
([#7212](elastic/eui#7212))

## [`88.4.0`](https://github.com/elastic/eui/tree/v88.4.0)

- Updated `EuiComboBox` to allow configuring text truncation behavior
via `truncationProps`. These props can be set on the entire combobox as
well as on on individual dropdown options.
([#7028](elastic/eui#7028))
- Updated `EuiInMemoryTable` with a new `searchFormat` prop (defaults to
`eql`). When setting this prop to `text`, the built-in search bar will
ignore EQL syntax and allow searching for plain strings with special
characters and symbols.
([#7175](elastic/eui#7175))

**Bug fixes**

- `EuiComboBox` now always shows the highlighted search text, even on
truncated text ([#7028](elastic/eui#7028))
- Fixed missing i18n in `EuiSearchBar`'s default placeholder and
aria-label text ([#7175](elastic/eui#7175))
- Fixed the inline compressed styles of `EuiDescriptionListTitle` to use
a taller line-height for readability
([#7185](elastic/eui#7185))
- Fixed `EuiComboBox` to correctly truncate selected items when
displayed as pills and plain text
([#7193](elastic/eui#7193))

**Accessibility**

- Added `aria-current` attribute to `EuiTablePagination`
([#7186](elastic/eui#7186))

**CSS-in-JS conversions**

- Converted `EuiDroppable` and `EuiDraggable` to Emotion; Removed
`$euiDragAndDropSpacing` Sass variables
([#7187](elastic/eui#7187))

---------

Co-authored-by: Patryk Kopycinski <[email protected]>
Co-authored-by: Jan Monschke <[email protected]>
Co-authored-by: Thomas Watson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants