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

[EuiTablePagination] Allow page size props to be configured via EuiProvider.componentProps #6951

Conversation

cee-chen
Copy link
Member

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

Summary

This PR is part 1 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).

The previous components configured to use the new defaults context were less complex to set up because they were undefined by default; this is the first component we're setting up where we want to fall back to an actual value.

The remaining work after this involves updating EuiBasicTable/EuiInMemoryTable/EuiDataGrid themselves to use the new default context, which you can preview here if you're super gung ho, but the diff was starting to get really large and unwieldy and I wanted to discuss some of the more general architectural choices without it muddying the waters.

I recommend following along by commit, as this PR contains a good amount of extra RTL test conversions/cleanup.

QA

General checklist

  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Added or updated jest and cypress tests
  • A changelog entry exists and is marked appropriately

- we're going to need this extra specificity shortly for when we add internal context defaults
@cee-chen cee-chen changed the title [EuiTablePagination] Set up inherited EUI defaults [EuiTablePagination] Allow itemsPerPage, itemsPerPageOptions, and showPerPageOptions to be configurabled via EuiProvider.componentProps Jul 15, 2023
@cee-chen cee-chen changed the title [EuiTablePagination] Allow itemsPerPage, itemsPerPageOptions, and showPerPageOptions to be configurabled via EuiProvider.componentProps [EuiTablePagination] Allow page size props to be configurabled via EuiProvider.componentProps Jul 15, 2023
- I played around with it, but unfortunately it doesn't makse sense to define the defaults in the component itself still. You either run into dependency issues (if you try to import them) or you have a lot of extra `??` repeated code if you try to make an extra layer of fallbacks
- convert tests to RTL

- remove unnecessary `() => {}` callback default (since prop is not required), use optional chaining instead
- default props must be manually declared using jsdocs now, since react docgen can no longer use `=` fallbacks to figure them out

- note: setting `EuiTablePagination.defaultProps` unfortunately does not work, React will automatically populate them and they don't come in as undefined
- PaginationBar should not be testing props that are basic pass-throughs - EuiTablePagination should (the whole point of unit tests)
+ add test for PaginationBar logic that's actually there (onPageChange)

- Move said missing tests to EuiTablePagination and improve tests to use specific assertions instead of noisy snapshots
@cee-chen cee-chen force-pushed the provider/table-pagination-1 branch from 23065c7 to 37ff1c3 Compare July 15, 2023 01:59
@cee-chen cee-chen changed the title [EuiTablePagination] Allow page size props to be configurabled via EuiProvider.componentProps [EuiTablePagination] Allow page size props to be configured via EuiProvider.componentProps Jul 15, 2023
@cee-chen cee-chen requested review from a team and tkajtoch July 15, 2023 02:12
@cee-chen cee-chen marked this pull request as ready for review July 15, 2023 02:12
onChangePage,
pageCount,
...rest
} = props;
Copy link
Member Author

Choose a reason for hiding this comment

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

Just want to quickly note that moving the = default parameters out of the initial function component means that our props table docgen can no longer automatically infer the default values, and we need to use manual jsdoc @default comments in our props documentation now like so:

* @default 50
*/
itemsPerPage?: number;
/**
* Custom array of options for "Rows per page".
* Pass `0` as one of the options to create a "Show all" option.
*
* @default [10, 20, 50, 100]

I'm not sure there's any way around this without coding up our own plugin or logic into our props doc generator - which I totally think is worth doing, but we should file that as a separate enhancement / a nice to have for another day. In the meanwhile, I think a little less DRYness for the sake of better docs/developer experience is worth it for now.

Copy link
Member Author

@cee-chen cee-chen Jul 25, 2023

Choose a reason for hiding this comment

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

Quick note to self: prop defaults docgen will remain as yet unresolved in this PR/feature branch

@kibanamachine
Copy link

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

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

This approach feels right on the money to me. I expanded my thoughts in thread.

Will change this to approved after other folks have had a chance to weigh in.

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@cee-chen cee-chen force-pushed the provider/table-pagination-1 branch from 0e45ed7 to 28523ea Compare July 25, 2023 04:09
@kibanamachine
Copy link

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

@cee-chen
Copy link
Member Author

cee-chen commented Jul 25, 2023

@tkajtoch Alrighty, I think this is ready for a final review if you don't mind taking another pass. Apologies for the more chaotic git history than normal, since we ended up going a bit back and forth on architecture 😅

I made some minor naming/copy tweaks, but the major change/cleanup item other than writing tests was I was able to move away from manually typing passed props somehow! 28523ea

Comment on lines +67 to +69
itemsPerPage = 50,
itemsPerPageOptions = [10, 20, 50, 100],
showPerPageOptions = true,
Copy link
Member Author

Choose a reason for hiding this comment

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

One quick note on this: I'm going to end up moving these defaults to a separate euiTablePaginationDefaults const/file in the next PR, but this is only because said defaults will end up being reused/referenced by EUI tables/datagrids, which is going to be a fairly unusual use-case for most components.

I would anticipate 99% of components w/ defaults to set them like the above, hence why I kept this as-is for this PR.

@kibanamachine
Copy link

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

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.

So excited about this one! The changes are looking great

@cee-chen cee-chen merged commit b46e557 into elastic:feature/provider/component-defaults Jul 25, 2023
@cee-chen cee-chen deleted the provider/table-pagination-1 branch July 25, 2023 20:56
cee-chen added a commit to cee-chen/eui that referenced this pull request Jul 25, 2023
…rovider.componentProps` (elastic#6951)

Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
cee-chen added a commit to cee-chen/eui that referenced this pull request Jul 25, 2023
…rovider.componentProps` (elastic#6951)

Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
cee-chen added a commit to cee-chen/eui that referenced this pull request Jul 26, 2023
…rovider.componentProps` (elastic#6951)

Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
cee-chen added a commit that referenced this pull request Aug 1, 2023
…rovider.componentProps` (#6951)

Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
cee-chen added a commit to cee-chen/eui that referenced this pull request Aug 1, 2023
…rovider.componentProps` (elastic#6951)

Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
cee-chen added a commit to cee-chen/eui that referenced this pull request Aug 3, 2023
…rovider.componentProps` (elastic#6951)

Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants