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

[EuiBasicTable][EuiInMemoryTable][EuiDataGrid] Update to use EuiTablePagination component defaults #6993

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jul 25, 2023

Summary

This PR is part 2 of 2 of the work required to allow all EUI tables and grids to have their pagination page size configurable by consumer defaults (see elastic/kibana#56406 for more context).

⚠️ Note that there is still actual work that needs to be done within Kibana itself to take advantage of the new component defaults functionality:

  1. The top-level <EuiProvider> in each Kibana app needs to be configured with user-preferred itemsPerPage and/or itemsPerPageOptions
  2. Every single instance of EuiBasicTable, EuiInMemoryTable, and EuiDataGrid needs to be configured to not initially pass a defined pagination.pageSize/pagination.pageSizeOptions. See ab3016d for an example of this change

This PR is also the last PR in the provider/component-defaults feature branch - once this PR merges in, the final feature PR itself will be opened up against main 🚢

As always, I strongly recommend following along by commit because there's a lot of RTL test tech debt as well as docs changes/tweaks in here.

QA

  • gh pr checkout 6993
  • yarn && yarn start
  • Go to http://localhost:8030/#/tabular-content/data-grid and http://localhost:8030/#/tabular-content/in-memory-tables
  • Confirm the first tables/grids default to a page size of 10, and when clicked, show 10, 25, 50 page sizes
  • Open src-docs/src/views/app_context.js
  • On the <EuiProvider component, add componentDefaults={{ EuiTablePagination: { itemsPerPage: 20, itemsPerPageOptions: [10, 20, 0] }}}
  • Save and refresh the above pages
  • Confirm the first tables/grids on the page now default to a page size of 20 and show 10, 20, show all rows options

General checklist

  • Props have proper autodocs (using @default if default values are missing) and playground toggles
    - [ ] Added documentation
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • A changelog entry exists and is marked appropriately

- [ ] 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
- [ ] Checked Code Sandbox works for any docs examples
- [ ] Updated the Figma library counterpart

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cee-chen cee-chen force-pushed the provider/table-pagination-2 branch from ebd3bb3 to 4d2d7fa Compare July 25, 2023 23:32
@cee-chen cee-chen force-pushed the feature/provider/component-defaults branch from e76c8da to 1a3a35e Compare July 25, 2023 23:32
@cee-chen cee-chen force-pushed the provider/table-pagination-2 branch from 4d2d7fa to 0a18b85 Compare July 25, 2023 23:34
@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cee-chen cee-chen force-pushed the provider/table-pagination-2 branch from 8c72ddd to 8b89730 Compare July 26, 2023 01:17
@cee-chen cee-chen force-pushed the feature/provider/component-defaults branch from 1a3a35e to 92f1d8b Compare July 26, 2023 01:19
@cee-chen cee-chen force-pushed the provider/table-pagination-2 branch from 8b89730 to f514d30 Compare July 26, 2023 01:20
@kibanamachine
Copy link

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

2 similar comments
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cee-chen cee-chen force-pushed the provider/table-pagination-2 branch from f514d30 to 6a0202e Compare July 26, 2023 02:14
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cee-chen cee-chen requested a review from tkajtoch July 26, 2023 03:01
@cee-chen cee-chen marked this pull request as ready for review July 26, 2023 03:01
@cee-chen
Copy link
Contributor Author

@tkajtoch Sending this PR your way since you already have some context from the previous first part of this work/PR, but please let me know if you're busy and I can reassign to another dev!

@cee-chen cee-chen force-pushed the feature/provider/component-defaults branch from 92f1d8b to 839b68d Compare August 1, 2023 18:50
@cee-chen cee-chen force-pushed the provider/table-pagination-2 branch from 6a0202e to 6f9c4e7 Compare August 1, 2023 18:53
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

- will be required/used by tables & grids shortly

- hook is an extra syntactical sugar bonus that *should* only be needed by this specific table pagination use case
… table's defaults

- these higher level components are the ones actually using these defaults anyway; we should defer to current production usage
cee-chen added 10 commits August 1, 2023 12:43
- requires some class-specific syntax/workarounds
- pagination now requires the correct context provider, which our RTL render provides by default but Enzyme doesn't

- we might as well reduce our snapshot footprint and write more specific assertions while here (note: this isn't perfect/complete, but it's a good start)
- anything that gets the current visible row index (e.g. focus) needs pageSize propagated from defaults

+ update types to reflect that internal `pagination` references should have all required props
…e` / `pageSizeOptions` props

- there's a few left, e.g. virtualization, and the focus example, where custom page sizes makes sense, but overall most do not need to set a custom page size
@cee-chen cee-chen force-pushed the feature/provider/component-defaults branch from 839b68d to 8f11a68 Compare August 1, 2023 19:53
@cee-chen cee-chen force-pushed the provider/table-pagination-2 branch from 6f9c4e7 to 8b2e9a0 Compare August 1, 2023 19:53
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 1, 2023

jenkins test this

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 1, 2023

@tkajtoch Alrighty, this feature is finally rebased against latest main / the React 18 work and ready for review once again 😄 I'm hoping to get it merged in by next Tuesday's release, so no immediate rush, but I'd really appreciate your eyes on this before this Friday. Please let me know if that's something you have time for!

@@ -7,7 +7,8 @@
*/

import React from 'react';
import { shallow, mount } from 'enzyme';
import { fireEvent } from '@testing-library/react';
import { render } from '../../../test/rtl';
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Comment on lines -281 to -300
>
<span
class="euiContextMenu__itemLayout"
>
<span
class="euiContextMenu__icon"
color="inherit"
data-euiicon-type="check"
/>
<span
class="euiContextMenuItem__text"
>
50 rows
</span>
</span>
</button>
<button
class="euiContextMenuItem"
data-test-subj="tablePagination-100-rows"
type="button"
Copy link
Member

Choose a reason for hiding this comment

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

Is it expected that now the snapshot doesn't have a checked element?

Copy link
Contributor Author

@cee-chen cee-chen Aug 3, 2023

Choose a reason for hiding this comment

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

oooo, awesome catch on this. This is indeed a regression that I'll need to fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually sorry, I spoke too soon - I just QA'd again locally and the checks are correctly appearing in the UI.

For this snapshot, the check element didn't go away, it's still there next to 10 rows:

<span
class="euiContextMenu__icon"
color="inherit"
data-euiicon-type="check"
/>
<span
class="euiContextMenuItem__text"
>
10 rows
</span>

Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

The changes look and work great! 🚢

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 3, 2023

Thanks Tomasz!! There'll be one final PR after this (the feature branch itself) that I'll send your way, but it'll hopefully be super minimal in review! :)

@cee-chen cee-chen merged commit 6acdd1d into elastic:feature/provider/component-defaults Aug 3, 2023
@cee-chen cee-chen deleted the provider/table-pagination-2 branch August 3, 2023 21:35
cee-chen added a commit to cee-chen/eui that referenced this pull request Aug 3, 2023
cee-chen added a commit to elastic/kibana that referenced this pull request Aug 21, 2023
`v86.0.0`⏩`v87.1.0`

⚠️ The biggest set of type changes in this PR come from the breaking
change that makes `pageSize` and `pageSizeOptions` now optional props
for `EuiBasicTable.pagination`, `EuiInMemoryTable.pagination` and
`EuiDataGrid.pagination`.

This caused several other components that were cloning EUI's pagination
type to start throwing type warnings about `pageSize` being optional.
Where I came across these errors, I modified the extended types to
require `pageSize`. These types and their usages may end up changing
again in any case once the Shared UX team looks into
#56406.

---

## [`87.1.0`](https://github.com/elastic/eui/tree/v87.1.0)

- Updated the underlying library powering `EuiAutoSizer`. This primarily
affects typing around the `disableHeight` and `disableWidth` props
([#6798](elastic/eui#6798))
- Added new `EuiAutoSize`, `EuiAutoSizeHorizontal`, and
`EuiAutoSizeVertical` types to support `EuiAutoSizer`'s now-stricter
typing ([#6798](elastic/eui#6798))
- Updated `EuiDatePickerRange` to support `compressed` display
([#7058](elastic/eui#7058))
- Updated `EuiFlyoutBody` with a new `scrollableTabIndex` prop
([#7061](elastic/eui#7061))
- Added a new `panelMinWidth` prop to `EuiInputPopover`
([#7071](elastic/eui#7071))
- Added a new `inputPopoverProps` prop for `EuiRange`s and
`EuiDualRange`s with `showInput="inputWithPopover"` set
([#7082](elastic/eui#7082))

**Bug fixes**

- Fixed `EuiToolTip` overriding instead of merging its
`aria-describedby` tooltip ID with any existing `aria-describedby`s
([#7055](elastic/eui#7055))
- Fixed `EuiSuperDatePicker`'s `compressed` display
([#7058](elastic/eui#7058))
- Fixed `EuiAccordion` to remove tabbable children from sequential
keyboard navigation when the accordion is closed
([#7064](elastic/eui#7064))
- Fixed `EuiFlyout`s to accept custom `aria-describedby` IDs
([#7065](elastic/eui#7065))

**Accessibility**

- Removed the default `dialog` role and `tabIndex` from push
`EuiFlyout`s. Push flyouts, compared to overlay flyouts, require manual
accessibility management.
([#7065](elastic/eui#7065))

## [`87.0.0`](https://github.com/elastic/eui/tree/v87.0.0)

- Added beta `componentDefaults` prop to `EuiProvider`, which will allow
configuring certain default props globally. This list of components and
defaults is still under consideration.
([#6923](elastic/eui#6923))
- `EuiPortal`'s `insert` prop can now be configured globally via
`EuiProvider.componentDefaults`
([#6941](elastic/eui#6941))
- `EuiFocusTrap`'s `crossFrame` and `gapMode` props can now be
configured globally via `EuiProvider.componentDefaults`
([#6942](elastic/eui#6942))
- `EuiTablePagination`'s `itemsPerPage`, `itemsPerPageOptions`, and
`showPerPageOptions` props can now be configured globally via
`EuiProvider.componentDefaults`
([#6951](elastic/eui#6951))
- `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid` now allow
`pagination.pageSize` to be undefined. If undefined, `pageSize` defaults
to `EuiTablePagination`'s `itemsPerPage` component default.
([#6993](elastic/eui#6993))
- `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid`'s
`pagination.pageSizeOptions` will now fall back to
`EuiTablePagination`'s `itemsPerPageOptions` component default.
([#6993](elastic/eui#6993))
- Updated `EuiHeaderLinks`'s `gutterSize` spacings
([#7005](elastic/eui#7005))
- Updated `EuiHeaderAlert`'s stacking styles
([#7005](elastic/eui#7005))
- Added `toolTipProps` to `EuiListGroupItem` that allows customizing
item tooltips. ([#7018](elastic/eui#7018))
- Updated `EuiBreadcrumbs` to support breadcrumbs that toggle popovers
via `popoverContent` and `popoverProps`
([#7031](elastic/eui#7031))
- Improved the contrast ratio of disabled titles within `EuiSteps` and
`EuiStepsHorizontal` to meet WCAG AA guidelines.
([#7032](elastic/eui#7032))
- Updated `EuiSteps` and `EuiStepsHorizontal` to highlight and provide a
more clear visual indication of the current step
([#7048](elastic/eui#7048))

**Bug fixes**

- Single uses of `<EuiHeaderSectionItem side="right" />` now align right
as expected without needing a previous `side="left"` sibling.
([#7005](elastic/eui#7005))
- `EuiPageTemplate` now correctly displays `panelled={true}`
([#7044](elastic/eui#7044))

**Breaking changes**

- `EuiTablePagination`'s default `itemsPerPage` is now `10` (was
previously `50`). This can be configured through
`EuiProvider.componentDefaults`.
([#6993](elastic/eui#6993))
- `EuiTablePagination`'s default `itemsPerPageOptions` is now `[10, 25,
50]` (was previously `[10, 20, 50, 100]`). This can be configured
through `EuiProvider.componentDefaults`.
([#6993](elastic/eui#6993))
- Removed `border` prop from `EuiHeaderSectionItem` (unused since
Amsterdam theme) ([#7005](elastic/eui#7005))
- Removed `borders` object configuration from `EuiHeader.sections`
([#7005](elastic/eui#7005))

**CSS-in-JS conversions**

- Converted `EuiHeaderAlert` to Emotion; Removed unused
`.euiHeaderAlert__dismiss` CSS
([#7005](elastic/eui#7005))
- Converted `EuiHeaderSection`, `EuiHeaderSectionItem`, and
`EuiHeaderSectionItemButton` to Emotion
([#7005](elastic/eui#7005))
- Converted `EuiHeaderLinks` and `EuiHeaderLink` to Emotion; Removed
`$euiHeaderLinksGutterSizes` Sass variables
([#7005](elastic/eui#7005))
- Removed `$euiHeaderBackgroundColor` Sass variable; use
`$euiColorEmptyShade` instead
([#7005](elastic/eui#7005))
- Removed `$euiHeaderChildSize` Sass variable; use `$euiSizeXXL` instead
([#7005](elastic/eui#7005))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>
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.

4 participants