[EuiTablePagination] Adding a `Show all` option; Plus Pagination Guidelines by cchaos · Pull Request #5547 · elastic/eui

cchaos · 2022-01-19T15:27:19Z

Guidelines

Can be found at /navigation/pagination/guidelines.

Internal only: Google doc for reference

EuiTablePagination

🔔 Breaking Change

This is a semi-trivial change, but I changed the name of the prop hidePerPageOptions to showPerPageOptions because I hate double-negative prop names. Boolean props should always be in the positive (with the exception of disabled since we inherit this from standard HTML). So that it's easier to read showPerPageOptions={false} vs hidePerPageOptions={true}.

Added the ability to present a "Show all" option by passing `'all'` in the `itemsPerPage` array

~~If 0 doesn't seem appropriate, I'm up for suggestions (maybe -1 is better).~~ But the idea is to not have to add yet another boolean prop but make it available as an option in the array. ~~Unless someone can help~~ @constancecchen helped me allow the term all into that prop's array as a different way to do this.

When this option is selected, it will hide the numbered pagination and present the button text as "Showing all".

Added a docs example for this component under Pagination / Table Pagination

With props tab and playground.

Other small fixes

Fixed layout of EuiTablePagination when it's container is narrow

Before the layout added scrollbars, but the problem is that this visually prioritizes the "Rows per page" instead of the actual pagination numbers.

After the layout has wrap on it, so without having to monitor the actual size of the container, if it's too narrow the pagination numbers will just wrap below the "Rows per page" first, then scroll if still necessary

Fixed EuiImage images from staying restricted to it's parent container's width, by adding `max-width: 100%`:

Before

After

Checklist

…rows’` option for `compressed`

…pdating the other `arrow` icons to a thicker weight

…nation

…customizable

…nation # Conflicts: # CHANGELOG.md # src/components/icon/__snapshots__/icon.test.tsx.snap

[Breaking] Changed `hidePerPageOptions` to the positive `showPerPageOptions`

cchaos

This PR is now ready for review while I finish up the PR checklist. 😄

src/components/table/table_pagination/table_pagination.tsx

cchaos · 2022-01-19T16:02:52Z

src/components/table/table_pagination/table_pagination.tsx

-      <EuiFlexGroup
-        justifyContent="spaceBetween"
-        alignItems="center"
-        responsive={false}


I removed responsive={false} to better ensure that the items wrap on small screens. Still not 100% effective for responsive behavior, but better than getting cutoff.

Ok, I think I've solved this even better:

Before the layout added scrollbars, but the problem is that this visually prioritizes the "Rows per page" instead of the actual pagination numbers.

After the layout has wrap on it, so without having to monitor the actual size of the container, if it's too narrow the pagination numbers will just wrap below the "Rows per page" first, then scroll if still necessary

src-docs/src/views/tables/paginated/paginated.js

kibanamachine · 2022-01-19T16:54:18Z

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

Fix dark mode in guidelines

kibanamachine · 2022-02-11T23:06:30Z

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

cee-chen

Apologies for the lateness, I planned on finishing reviewing last Friday after my eye exam but then my pupils got dilated and my near vision was shot for a few hours after 🙈 I just have some some super minor type-related feedback, everything else looks great!

src/components/basic_table/pagination_bar.tsx

src/components/table/table_pagination/table_pagination.tsx

…tion

cchaos · 2022-02-15T19:18:26Z

This PR is ready for final review. Thanks so much @constancecchen for addition of the 'all' option. Much clearer intent on how to add this as an option over 0.

@1Copenut I'd love you a11y evaluation over how this affects the overall table screen reader functionality. You can see a full example under the Preserve the user-customized state of pagination section of the guidelines page https://eui.elastic.co/pr_5547/#/navigation/pagination/guidelines

kibanamachine · 2022-02-15T19:52:20Z

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

kibanamachine · 2022-02-15T20:50:04Z

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

1Copenut · 2022-02-15T22:04:57Z

@cchaos I took a look at the linked page, and spent a while considering language. I noticed the option 10 rows uses passive voice, where Show all uses active voice. That's not a problem, just an observation. It might be cool to have an accessible label that says "Show all rows" semantically, and retains the same visual label. Where all rows is the outlier(?) that removes pagination behavior, having it use a different voice might be a nice cue that the UI is going to change.

cee-chen

Love the wrap change! This LGTM to me from a code POV, although I have a few small perf suggestions. If you're OK with them I can push up the memoization hooks to this branch, up to you

src/components/table/table_pagination/table_pagination.tsx

cchaos · 2022-02-16T17:48:05Z

If you're OK with them I can push up the memoization hooks to this branch, up to you

Yes please!!! 🙏

kibanamachine · 2022-02-17T03:39:45Z

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

src/components/table/table_pagination/table_pagination.tsx

cchaos · 2022-02-18T20:49:26Z

@1Copenut Based on Gail's recommendation for the button text and your suggestion for screen-reader text, I've just gone ahead and updated the option's visible text to include the word rows. Having specific SR text for this doesn't seem necessary. Everyone can benefit from the extra/precise language.

kibanamachine · 2022-02-18T21:26:28Z

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

elizabetdev

Thanks for fixing the issues.

I read the guidelines and tested the changes again. LGTM! 🎉

cchaos added 26 commits November 1, 2021 15:25

Fix console error of Custom table example

756d2e4

[EuiPagination] Allowing null if pageCount is unknown

86a36a1

[EuiPagination] Added tooltip around prev/next buttons and added `'ar…

faf9b6d

…rows’` option for `compressed`

[EuiIcon] Added doubleArrowLeft and doubleArrowRight while also u…

ddf3940

…pdating the other `arrow` icons to a thicker weight

Merge remote-tracking branch 'upstream/main' into feature/simple_pagi…

95875ef

…nation

Added first and last buttons and refactored arrow button component

57ef10d

Using negative index for activePage to indicate last page

46476e3

Cleanup component

cedef9c

Render first/last if compressed and remove clicks from numbers

46bfd8d

Update examples and add one for Indeterminate

c5d5ec3

Fix a11y

82fd090

Merge remote-tracking branch 'upstream/main' into feature/simple_pagi…

e7d23c6

…nation

cl

fc129ba

Added responsive prop that renders the compressed display and is …

ff3542f

…customizable

cl

5227677

Merge remote-tracking branch 'upstream/main' into feature/simple_pagi…

8aa7e20

…nation # Conflicts: # CHANGELOG.md # src/components/icon/__snapshots__/icon.test.tsx.snap

Fix snippets

02e59e1

Initial text and images for Guidelines

a020fe9

Allowing 0 for pageCount and adding example of table saved state

f8fccde

Updating copy from UX writer review

479f27d

Converted EuiTablePagination to FunctionComponent

38c53d8

[EuiTablePagination] Extending all EuiPagination props

9144b37

[Breaking] Changed `hidePerPageOptions` to the positive `showPerPageOptions`

Merge remote-tracking branch upstream/main into guidelines/pagination

7e106e5

Update Guidelines per suggestions

aab8360

Fix snaps

06ec154

Cleanup

63b5ff2

cchaos commented Jan 19, 2022

View reviewed changes

[EuiImage] Fix max-width

34a0d86

Fix dark mode in guidelines

cchaos changed the title ~~[WIP] [EuiTablePagination] Adding a Show all option; Plus Pagination Guidelines~~ [EuiTablePagination] Adding a Show all option; Plus Pagination Guidelines Jan 19, 2022

cee-chen force-pushed the guidelines/pagination branch from 343dfdd to 62ab0cc Compare February 11, 2022 22:26

cee-chen reviewed Feb 14, 2022

View reviewed changes

src/components/basic_table/pagination_bar.tsx Outdated Show resolved Hide resolved

src/components/table/table_pagination/table_pagination.tsx Show resolved Hide resolved

cchaos added 3 commits February 15, 2022 14:02

Last few fixes

befab51

Merge remote-tracking branch '2_upstream/main' into guidelines/pagina…

9e35e86

…tion

cl

b3e2267

cchaos requested review from cee-chen and elizabetdev February 15, 2022 19:18

Merge branch 'main' into guidelines/pagination

57d9cfe

cee-chen approved these changes Feb 16, 2022

View reviewed changes

src/components/table/table_pagination/table_pagination.tsx Outdated Show resolved Hide resolved

src/components/table/table_pagination/table_pagination.tsx Outdated Show resolved Hide resolved

src/components/table/table_pagination/table_pagination.tsx Outdated Show resolved Hide resolved

[PR feedback] Performance optimization + fn naming suggestion

c6d3c4d

cchaos commented Feb 18, 2022

View reviewed changes

src/components/table/table_pagination/table_pagination.tsx Outdated Show resolved Hide resolved

cchaos and others added 4 commits February 18, 2022 15:36

Update src/components/table/table_pagination/table_pagination.tsx

a72972c

snaps

0fdfff1

Merge branch 'main' into guidelines/pagination

7d0a43c

Append rows to option text

c47e3b7

elizabetdev approved these changes Feb 22, 2022

View reviewed changes

cchaos merged commit 28e0a2b into elastic:main Feb 22, 2022

cchaos deleted the guidelines/pagination branch February 22, 2022 18:09

This was referenced Mar 4, 2022

Upgrade EUI to v51.1.0 elastic/kibana#126926

Merged

[EuiTablePagination] Incorrect type for itemsPerPage and itemsPerPageOptions depending on pagination.pageSizeOptions #5697

Closed

cee-chen mentioned this pull request Mar 8, 2022

[EuiBasicTable][EuiInMemoryTable][EuiDataGrid] Revert page size number | 'all' type to just number #5699

Merged

7 tasks

Conversation

cchaos commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Guidelines

EuiTablePagination

🔔 Breaking Change

Added the ability to present a "Show all" option by passing 'all' in the itemsPerPage array

Added a docs example for this component under Pagination / Table Pagination

Other small fixes

Fixed layout of EuiTablePagination when it's container is narrow

Fixed EuiImage images from staying restricted to it's parent container's width, by adding max-width: 100%:

Checklist

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cchaos Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Jan 19, 2022

Uh oh!

kibanamachine commented Feb 11, 2022

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cchaos commented Feb 15, 2022

Uh oh!

kibanamachine commented Feb 15, 2022

Uh oh!

kibanamachine commented Feb 15, 2022

Uh oh!

1Copenut commented Feb 15, 2022

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cchaos commented Feb 16, 2022

Uh oh!

kibanamachine commented Feb 17, 2022

Uh oh!

Uh oh!

cchaos commented Feb 18, 2022

Uh oh!

kibanamachine commented Feb 18, 2022

Uh oh!

elizabetdev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cchaos commented Jan 19, 2022 •

edited

Loading

Added the ability to present a "Show all" option by passing `'all'` in the `itemsPerPage` array

Fixed EuiImage images from staying restricted to it's parent container's width, by adding `max-width: 100%`: