feat: add pagination nav component #6199

janhassel · 2020-06-04T12:48:35Z

React implementation of PaginationNav component. https://the-carbon-components.netlify.app/?nav=pagination-nav

@joshblack We talked about this one a while ago, especially the dynamic behaviour of the overflow. I opened this as a draft PR so we can get a discussion started on what you guys need from us for this contribution.

This PR comes with two UX teaks in comparison to the current vanilla component.

It adds a "loop" prop (disabled by default) that allows users to cycle through all pages when they reach the end / start.

The behaviour of how the overflow is treated and presented to the user changes. Instead of overlaying the overflow with a page indicator, the component always keeps the current page visible and out of the overflow (see screen recordings attached)

Current behaviour:
Proposed behaviour:

netlify · 2020-06-04T13:01:54Z

Deploy preview for carbon-elements ready!

Built with commit 0b80936

https://deploy-preview-6199--carbon-elements.netlify.app

netlify · 2020-06-04T13:09:40Z

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 0b80936

https://deploy-preview-6199--carbon-components-react.netlify.app

emyarod · 2020-06-04T14:05:58Z

I've had this completed for almost 9 months now, including the overflow behavior but since it was deprioritized, all I have left is the test suite. if we're ready to bring this into the library I can just round out the test suite and open a PR

packages/react/src/components/PaginationNav/PaginationNav.js

joshblack · 2020-06-10T18:15:54Z

@aagonzales gave it a look and really liked the changes! I think we could bring in the UX changes along with the code formatting stuff mentioned above (along with style guide stuff). Feel free to use any of the existing work too and we can definitely help ya out 😄

If you have any questions about tests btw, just let me know!

janhassel · 2020-06-15T09:29:48Z

@joshblack Thanks for the feedback (and the addition of translateWithId in the styleguide). I refactored the component accordingly. Would you mind taking another look and if there are only minor things left, I can work on the test suite.

joshblack

Looking great! 🎉 Just had a couple of questions:

What should the responsive behavior be like for this component?
Do we need to flag the current page with aria-current="page"?

For determining cuts, it seems like it works great at the midpoint but when hitting past that it seems to make what is clicked jump to the edge before the overflow:

Would it make sense to have that midpoint behavior happen throughout the range of pages?

joshblack · 2020-06-16T19:30:25Z

packages/react/src/components/PaginationNav/PaginationNav.js

+ <select
+ className={`${prefix}--pagination-nav__page ${prefix}--pagination-nav__page--select`}
+ aria-label={`Select ${t('carbon.pagination-nav.item')} number`}
+ onChange={e => {


Should we be using onBlur here per the jsx-a11y rule?

The issue with onBlur is that it only triggers once the user clicks on another element or continues with tab

janhassel · 2020-06-19T15:36:17Z

@joshblack

I updated (and actually simplified) the overflow behaviour to feel more natural. Since I haven't worked with hooks very much I'd really appreciate your input on my implementation there since I suppressed the warnings from React about the dependency array and I'm not sure if there is another way for the functionality here.
Regarding aria-current="page": good point! Missed that, thanks!
For the responsive behaviour our team is planning to always maximize it to width available. The exact calculation would probably depend on the product though. So passing in the maximum amount of pages to props.itemsShown would be my proposal. Btw, the component enforces a minimum of 4 shown items. Together with the two buttons that's 288px (6*48) width which fits perfectly in the smallest screen size supported (288px component + $spacing-05 left and $spacing-05 right = 320px)

joshblack · 2020-06-29T14:31:04Z

Hey @janhassel! 👋 So glad that this got moved from draft -> ready for review 🎉

I wanted to ask you if you had an open slot sometime this week for us to go over the PR on a call? Thought it might be helpful for all of us and hopefully speed up the process of getting things merged in 😄 This really would only be talking through the component, its intended behaviors, any relevant implementation details you think are worth sharing, etc. Totally understand if not! Just wanted to reach out and see.

janhassel · 2020-06-29T14:50:31Z

@joshblack Definitely! I sent you an invite for tomorrow morning, feel free to counter if you prefer a different time slot 🙂

…hown

aledavila

This is awesome work. So far it looks good

Good to go:
MacOS: Chrome, Firefox, and Safari. No DAP violations. Keyboard navigation works.

Issues on VO:
Safari:
When selecting a page it doesn't announce if I have selected it. If I am navigating via arrows it does not read which page I am highlighting.

dakahn

Found a few things testing with JAWS 2020 and NVDA on Firefox latest in Windows 10:

the active button has no focus state
the current active page button is described as "unavailable" -- which makes sense technically, but sounds quite confusing
when the user selects a page the button's label should be updated to 'active'
the next page button when activated doesn't announce the changes (updated page from previous page)

janhassel · 2020-07-08T13:14:42Z

Thanks for the feedback!

@aledavila

Should be fixed now

@dakahn

it's in now but it doesn't look too good due to the blue underline. Maybe @aagonzales can give some visual guidance here?
This is an interesting one as I couldn't provoke a similar behaviour on VO (the only machine with Windows and JAWS installed is in the office, so I currently don't have access to that one). Is this how JAWS usually describes a disabled button (similar to "dimmed" on VO) or what could cause that?
I updated the label. Let me know if it is better now
Should be fixed

I'm currently facing the problem that the live region "Page 5 of 10" is read twice and after some poking around it seems that the component is rendering twice when a button is clicked. However, I cannot see this in the React DevTools Profiler.
Do you guys have any idea what could be causing this (or is it maybe only an issue with my setup)?

aagonzales · 2020-07-08T18:16:00Z

Hi @janhassel ! I talked to Jeannie about the selected+focus state overlap and she says its fine. We agree it does look not the best but we can live with it since its a temporary state and follows the correct styles.

aledavila

Thanks for the updates! LGTM pending @dakahn changes

dakahn · 2020-07-08T21:47:58Z

We're reading button updates now with NVDA which is great. When the 'next page button' is activated you just here '5', '6' ,'7' etc maybe something more clear might be, "page 5, active", but this isn't a blocker.

Unavailable is what's read by JAWS and NVDA in place of dimmed -- it's not a blocker technically since it's correct.

~~JAWS 2020 still isn't reading the page updates when the 'next page button' is activated. Let me see if I can get this working for you since I can test on JAWS here on my machine~~

dakahn

We're actually good to go here. JAWS is reading aria-live updates just fine. (fun fact, JAWS will not read aria-live updates unless it is started and running before opening Firefox -- I'm talking to you, future me)

janhassel · 2020-07-09T07:37:29Z

@dakahn That's good to know actually, might save me a lot of time debugging once I have access to the JAWS machine again 😀

I added aria-atomic to the live region which should cause JAWS to read out the full status now ("Page 5 of 10") instead of just the number. I didn't notice a difference on Chrome or Safari with VO, hence I didn't include it earlier. Can you give it a try and see if it's as expected now?

janhassel added 3 commits June 4, 2020 09:32

feat: add pagination nav component

39b8730

Merge branch 'master' into pagination-nav-react

863bb9c

fix(pagination-nav): fix prettier issues

7feac7a

joshblack reviewed Jun 4, 2020

View reviewed changes

packages/react/src/components/PaginationNav/PaginationNav.js Outdated Show resolved Hide resolved

joshblack reviewed Jun 4, 2020

View reviewed changes

packages/react/src/components/PaginationNav/PaginationNav.js Outdated Show resolved Hide resolved

joshblack reviewed Jun 4, 2020

View reviewed changes

packages/react/src/components/PaginationNav/PaginationNav.js Outdated Show resolved Hide resolved

janhassel added 2 commits June 15, 2020 10:52

refactor(pagination-nav): convert to functional component

ec4a09b

refactor(pagination-nav): replace text props with translateWithId

b5607bd

janhassel requested a review from joshblack June 15, 2020 14:44

joshblack reviewed Jun 16, 2020

View reviewed changes

janhassel added 2 commits June 19, 2020 17:22

fix(pagination-nav): improve overflow behaviour

364a05e

fix(pagination-nav): add aria-current to active page

6f6cbbc

janhassel added 2 commits June 19, 2020 17:50

Merge branch 'master' into pagination-nav-react

c1034c5

test(pagination-nav): add tests

8615a10

janhassel marked this pull request as ready for review June 26, 2020 12:00

janhassel requested a review from a team as a code owner June 26, 2020 12:00

ghost requested review from aledavila and dakahn June 26, 2020 12:00

janhassel and others added 3 commits June 30, 2020 13:54

fix(pagination-nav): fix overflow behavour when only 4 items can be s…

038942e

…hown

Merge branch 'master' into pagination-nav-react

108b11e

Merge branch 'master' into pagination-nav-react

c80f9de

aledavila suggested changes Jul 7, 2020

View reviewed changes

dakahn suggested changes Jul 7, 2020

View reviewed changes

janhassel added 6 commits July 8, 2020 09:51

chore(pagination-nav): add prop types to internal components

c71033d

fix(pagination-nav): add focus state to active page

2aace9c

fix(pagination-nav): add "active" to accessibility label

8eadc46

chore(pagination-nav): fix prettier issue

bf563bb

Merge branch 'master' into pagination-nav-react

a2a9aac

fix(pagination-nav): announce active page when using direction buttons

b3f6af8

janhassel requested review from aledavila and dakahn July 8, 2020 13:14

fix(pagination-nav): fix unnecessary re-render for first and last page

e07b4ee

aledavila approved these changes Jul 8, 2020

View reviewed changes

Merge branch 'master' into pagination-nav-react

e73d88f

dakahn approved these changes Jul 8, 2020

View reviewed changes

dakahn and others added 2 commits July 8, 2020 18:37

Merge branch 'master' into pagination-nav-react

48e2b2f

fix(pagination-nav): read full state after page change

4b95a2a

joshblack added the status: ready to merge 🎉 label Jul 9, 2020

kodiakhq bot added 2 commits July 9, 2020 23:29

Merge branch 'master' into pagination-nav-react

4fddad0

Merge branch 'master' into pagination-nav-react

0b80936

kodiakhq bot merged commit 007b1e6 into carbon-design-system:master Jul 10, 2020

laurenmrice mentioned this pull request Jul 13, 2020

feat:(Pagination): Stand alone Small pagination #2634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add pagination nav component #6199

feat: add pagination nav component #6199

janhassel commented Jun 4, 2020 •

edited

Loading

netlify bot commented Jun 4, 2020 •

edited

Loading

netlify bot commented Jun 4, 2020 •

edited

Loading

emyarod commented Jun 4, 2020 •

edited

Loading

joshblack commented Jun 10, 2020

janhassel commented Jun 15, 2020

joshblack left a comment

joshblack Jun 16, 2020

janhassel Jun 19, 2020

janhassel commented Jun 19, 2020 •

edited

Loading

joshblack commented Jun 29, 2020

janhassel commented Jun 29, 2020

aledavila left a comment

dakahn left a comment

janhassel commented Jul 8, 2020

aagonzales commented Jul 8, 2020

aledavila left a comment

dakahn commented Jul 8, 2020 •

edited

Loading

dakahn left a comment

janhassel commented Jul 9, 2020

feat: add pagination nav component #6199

feat: add pagination nav component #6199

Conversation

janhassel commented Jun 4, 2020 • edited Loading

netlify bot commented Jun 4, 2020 • edited Loading

netlify bot commented Jun 4, 2020 • edited Loading

emyarod commented Jun 4, 2020 • edited Loading

joshblack commented Jun 10, 2020

janhassel commented Jun 15, 2020

joshblack left a comment

Choose a reason for hiding this comment

joshblack Jun 16, 2020

Choose a reason for hiding this comment

janhassel Jun 19, 2020

Choose a reason for hiding this comment

janhassel commented Jun 19, 2020 • edited Loading

joshblack commented Jun 29, 2020

janhassel commented Jun 29, 2020

aledavila left a comment

Choose a reason for hiding this comment

dakahn left a comment

Choose a reason for hiding this comment

janhassel commented Jul 8, 2020

aagonzales commented Jul 8, 2020

aledavila left a comment

Choose a reason for hiding this comment

dakahn commented Jul 8, 2020 • edited Loading

dakahn left a comment

Choose a reason for hiding this comment

janhassel commented Jul 9, 2020

janhassel commented Jun 4, 2020 •

edited

Loading

netlify bot commented Jun 4, 2020 •

edited

Loading

netlify bot commented Jun 4, 2020 •

edited

Loading

emyarod commented Jun 4, 2020 •

edited

Loading

janhassel commented Jun 19, 2020 •

edited

Loading

dakahn commented Jul 8, 2020 •

edited

Loading