[EuiPagination] Added `first`/`last` buttons for compressed and allowing for indeterminate `pageCount` #5362

cchaos · 2021-11-08T23:45:50Z

Don't look at the commit summaries, they're inaccurate with the end result 😆

This one got away from me a bit, which meant doing a bit of refactor of the component itself. I'll highlight the actual changes:

Added "First page" and "Last page" arrow buttons when `compressed=true`

I also removed the click handlers the numbers completely to just rely on the arrow buttons for navigation and the numbers for indicating current page.

Screen.Recording.2021-11-08.at.06.41.03.PM.mp4

This also fixes the situation where users could never get back to the first page easily because the "first / 1" button was replaced with the current page.

Before

Screen.Recording.2021-11-08.at.06.42.07.PM.mp4

Added `responsive` prop that changes the mobile view to `compressed` and is customizable

Before, the simplicity in mobile views was ok, but I think it lacked context. So I removed the custom Sass mixins and just created a responsive prop (with the same breakpoints as before) and forced to the compressed view if responsive is anything but `false.

Handle situation where `pageCount=0` as indeterminate

Fixes #4506

This meant generally hiding all the numbered buttons and relying solely on the arrow buttons to navigate. In order to simplify the activePage count, I determined that negative indexes start from the end. So when pageCount=0 and the user clicks the "Last page" button, it will return -1.

Screen.Recording.2021-11-08.at.06.37.14.PM.mp4

Added `doubleArrowLeft`, `doubleArrowRight`, `arrowStart`, `arrowEnd` to EuiIcon

These didn't exist yet in order to create the first/last buttons. I also updated the regular arrow versions to the slightly fatter version we've been using in Figma.

Update: I replaced the double arrows below with the new ones above, but kept the double arrows in for any future use cases.

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

…rows’` option for `compressed`

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

…nation

cchaos

I'm keeping this in draft because I want to add some guidelines based on this comment #4506 (comment) but the component work should be reviewable at this point.

cchaos · 2021-11-08T23:48:20Z

src/components/pagination/pagination_button_arrow.tsx

+  const label = useEuiI18n(
+    'euiPaginationButtonArrow.previousPage',
+    ({ page }) => `${toSentenceCase(type)} page${page ? `, ${page}` : ''}`,
+    {
+      page: labelModifier,
+    }
+  );


I was trying to be clever here and interpolate the type into the i18n, but I'm not sure this is going to work properly. I'm open to suggestions to fix.

It probably makes the most sense to be verbose and have four distinct labels, one for each type. In any case the token name will need to be updated to match (euiPaginationButtonArrow.previousPage is only correct sometimes)

Yeah I just seem to constantly run into the "Hooks can be conditional" issue.

Maybe do:

const labels = { first: useEuiI18n(...), previous: useEuiI18n(...), next: useEuiI18n(...), last: useEuiI18n(...) } const label = labels[type];

Does it make sense to calculate those on every button mount when they'll each only use one?

Well I did that method anyway, just for the type part: 043654f

kibanamachine · 2021-11-09T00:09:57Z

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

…nation

kibanamachine · 2021-11-09T21:02:25Z

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

…customizable

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

cchaos · 2021-11-11T16:48:37Z

I've decided to push off the Guidelines to a follow up PR because it may require some work to the EuiTablePagination component as well. So I'm opening this PR up for review now.

kibanamachine · 2021-11-11T17:00:42Z

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

kibanamachine · 2021-11-11T17:32:14Z

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

kibanamachine · 2021-11-15T22:11:08Z

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

thompsongl · 2021-11-17T16:12:57Z

So when pageCount=0 and the user clicks the "Last page" button, it will return -1.

This makes sense, but is this the intended result if you continue changing the pagination?

activePage continues using -1 and decrements from there. This is probably understandable for devs implementing the resulting ES query, but I'm not sure it makes sense to show the user a negative number.

cchaos · 2021-11-17T16:18:58Z

but I'm not sure it makes sense to show the user a negative number.

I had thought about that too, but honestly, I think it's better to indicate something about their current state in the pagination than nothing.

cchaos · 2021-11-23T19:12:56Z

Woot! I've pushed the final SR-only wrapper. Can you take one last look once that builds to make sure it all still gets read out properly? It also adds that <span> you had with all the aria- attributes. I wasn't sure if that's still needed or not. d96e254

kibanamachine · 2021-11-23T19:43:39Z

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

elizabetdev · 2021-11-23T19:50:48Z

@cchaos it's looking good!

I found just a few small issues in Firefox (94.0.2) and I have one design suggestion. I tested in Chrome, Safari, Edge, and Firefox.

Firefox issues

Focus rings now is being cut in Firefox.

The hover styles are not taking effect in Firefox.

First and last page icon

I did a quick research on other pagination components and most of them are using a similar icon to the ones you added: doubleArrowLeft and doubleArrowRight. But IMO the following type of icon represents better first and last pages. What do you think?

cchaos · 2021-11-23T21:01:55Z

Thanks @miukimiu . I'm working on those Firefox fixes.

In general, first/last buttons aren't great but it's the only way to quickly get to the first page when there isn't numbering. I also agree that in isolation, the icon that you showed is more representative of going to the beginning/end. The problem I see with them is that they harder to distinguish between themselves and the single arrow previous/next icons.

If you'd like to continue exploring options for the icon, I'm happy to wait to finalize this PR next week.

kibanamachine · 2021-11-23T21:39:52Z

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

1Copenut · 2021-11-24T13:00:23Z

src/components/pagination/pagination.tsx

+  // The compressed version already should the activePage with pageCount, so it just needs `Page`
+  // The other types will append the whole string with total pageCount or `collection`
+  const accessiblePageCount = compressed
+    ? pageLabel


The compressed true statement renders "Page" in the compressed SR-only block. Could we add a dynamic element like activePage + 1 so screen readers pick up the change and announce a page number?

This is my only additional comment. The code is working really well in VO and I was able to traverse each pagination example easily and clearly.

Would that not duplicate the active page number and make the readout "Page 1, 1 of 5"?

Reading my comment again, I don't think I was as clear as I'd hoped.

The compressed accessiblePageCount text is just "Page" right now. For consistency, I'd like to make it render "Page 1 of 5". We should be able to pull that off with string interpolation like you're using in other SR-only blocks.

! pagination.tsx#289 const pageLabel = useEuiI18n('euiPagination.page', 'Page');

I mostly understood what you meant. But I'm wondering if that will duplicate what the screen-reader reads out since the actual visible text is 1 of 5 but the screen-reader-only text would be Page 1 of 5. So would it not then read both? So you'd hear "Page 1 of 5. 1 of 5"?

Okay, I'm following you now. Yes and no. The visual text would be a repeat, but that change wouldn't be announced when users press Next, Previous, First, or Last buttons. Screen reader users will just hear the SR-only live region we're adding until they arrow through the visual text. If they're moving by TAB or other shortcut keys, they may never settle on the visual text. It could feel a little heavy, but I'm inclined to err on the side of redundancy over brevity in this case.

Ok gotcha, I'll just remove the compressed specific logic and have it read out the same as all the others.

1Copenut

Great work! I had just one comment about the compressed view's SR-only message, and I'm ready to approve.

…nation

elizabetdev · 2021-12-02T17:02:04Z

Before, the simplicity in mobile views was ok, but I think it lacked context

@cchaos I agree with this and I prefer having more context. But just noticed that now in mobile the pagination is taking too much space. Is there anything we can do to reduce the size? Maybe in mobile instead of saying "Rows per page" just "Rows"?

kibanamachine · 2021-12-02T17:22:03Z

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

elizabetdev

Thanks @cchaos for fixing the Firefox issues. LGTM! 🎉

Because we also have to rethink the pagination for #4458 (smarter pagination display on skinny screens/containers) I'm ok if you ignore my latest comment.

1Copenut

👍 LGTM!

…ination buttons

cchaos · 2021-12-02T19:57:46Z

Thanks @1Copenut and @miukimiu !!! I've pushed the new icons:

As for the mobile issues with the table pagination that contains the Rows per page selector, I will be dealing with that in a follow up I've already started to help in general with that pattern.

kibanamachine · 2021-12-02T20:22:41Z

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

cchaos · 2021-12-03T20:55:30Z

I'd love to get a final eng check on this PR before I merge 🙏

thompsongl

Code changes LGTM!

cchaos · 2021-12-03T21:48:37Z

Merging! Thanks everyone for the reviews 💟

cchaos added 10 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

cchaos commented Nov 8, 2021

View reviewed changes

cchaos mentioned this pull request Nov 9, 2021

[EuiPagination] add a variation with next/previous buttons only #4506

Closed

cchaos added 3 commits November 9, 2021 15:29

Fix a11y

82fd090

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

e7d23c6

…nation

cl

fc129ba

cchaos added 3 commits November 11, 2021 10:50

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

cchaos marked this pull request as ready for review November 11, 2021 16:48

Fix snippets

02e59e1

cchaos and others added 2 commits November 15, 2021 16:27

Merge branch 'elastic:main' into feature/simple_pagination

4b0194f

i18n updates

043654f

Wrapping with ScreenReaderOnly

d96e254

Merge branch 'main' into feature/simple_pagination

fa17c54

cchaos requested review from 1Copenut and thompsongl November 23, 2021 21:06

Fix focus rings cutoff by overflow: hidden

83fe69f

1Copenut reviewed Nov 24, 2021

View reviewed changes

cchaos added 2 commits December 2, 2021 11:21

Remove compressed specific SR text

12bc622

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

f8629dc

…nation

cchaos requested a review from 1Copenut December 2, 2021 16:57

elizabetdev approved these changes Dec 2, 2021

View reviewed changes

1Copenut approved these changes Dec 2, 2021

View reviewed changes

Added arrowStart and arrowEnd icons to replace the first/last pag…

52c9289

…ination buttons

thompsongl approved these changes Dec 3, 2021

View reviewed changes

cchaos merged commit c25a2c2 into elastic:main Dec 3, 2021

cchaos deleted the feature/simple_pagination branch December 3, 2021 21:52

thompsongl mentioned this pull request Dec 9, 2021

Upgrade EUI to v43.1.1 elastic/kibana#120935

Merged

1 task

criamico mentioned this pull request Jan 5, 2022

[Fleet]: Screenshots column is broken under integration overview page. elastic/kibana#122202

Closed

criamico mentioned this pull request Feb 8, 2022

[Fleet] Remove unwanted overflow on Integrations screenshots elastic/kibana#124975

Merged

[EuiPagination] Added first/last buttons for compressed and allowing for indeterminate pageCount #5362

[EuiPagination] Added first/last buttons for compressed and allowing for indeterminate pageCount #5362

Conversation

cchaos commented Nov 8, 2021 • edited Loading

Don't look at the commit summaries, they're inaccurate with the end result 😆

Added "First page" and "Last page" arrow buttons when compressed=true

Added responsive prop that changes the mobile view to compressed and is customizable

Handle situation where pageCount=0 as indeterminate

Added doubleArrowLeft, doubleArrowRight, arrowStart, arrowEnd to EuiIcon

Checklist

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Nov 9, 2021

kibanamachine commented Nov 9, 2021

cchaos commented Nov 11, 2021

kibanamachine commented Nov 11, 2021

kibanamachine commented Nov 11, 2021

kibanamachine commented Nov 15, 2021

thompsongl commented Nov 17, 2021

cchaos commented Nov 17, 2021

cchaos commented Nov 23, 2021

kibanamachine commented Nov 23, 2021

elizabetdev commented Nov 23, 2021 • edited Loading

Firefox issues

First and last page icon

cchaos commented Nov 23, 2021

kibanamachine commented Nov 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1Copenut Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1Copenut left a comment

Choose a reason for hiding this comment

elizabetdev commented Dec 2, 2021

kibanamachine commented Dec 2, 2021

elizabetdev left a comment

Choose a reason for hiding this comment

1Copenut left a comment

Choose a reason for hiding this comment

cchaos commented Dec 2, 2021

kibanamachine commented Dec 2, 2021

cchaos commented Dec 3, 2021

thompsongl left a comment

Choose a reason for hiding this comment

cchaos commented Dec 3, 2021

[EuiPagination] Added `first`/`last` buttons for compressed and allowing for indeterminate `pageCount` #5362

[EuiPagination] Added `first`/`last` buttons for compressed and allowing for indeterminate `pageCount` #5362

cchaos commented Nov 8, 2021 •

edited

Loading

Added "First page" and "Last page" arrow buttons when `compressed=true`

Added `responsive` prop that changes the mobile view to `compressed` and is customizable

Handle situation where `pageCount=0` as indeterminate

Added `doubleArrowLeft`, `doubleArrowRight`, `arrowStart`, `arrowEnd` to EuiIcon

cchaos Nov 15, 2021 •

edited

Loading

elizabetdev commented Nov 23, 2021 •

edited

Loading

1Copenut Nov 30, 2021 •

edited

Loading