cengage / react-magma

https://react-magma.cengage.com
MIT License
21 stars 12 forks source link

New component: Pagination variant #991

Closed orion-cengage closed 1 year ago

orion-cengage commented 1 year ago

Purpose: We need a new variant on the existing pagination component. It will be a better option for all-sized screens when you can expect more than 5 or 6 pages of results.

Figma: https://www.figma.com/file/cPCfDVWC38T7Qt3dIL95sR/Components---Navigation?node-id=1%3A2&t=5s8dkFy7tdXHpYLp-1

AC

1.0 Component has two buttons that navigate to the Previous Page and Next Page. -- 1.1 The Previous Page button is disabled when you're on the first page. -- 1.2 The Next Page button is disabled when you're on the last page. -- 1.3 Put tooltips on both buttons with labels "Previous page" and "Next page". 2.0 Display the page you're currently on, as well as choose a specific page, using the Native Select component. 3.0 Display the number of pages using a text label. 4.0 The width of the component is dependent on the container it is inside of. But if the component gets too wide, the integrity of the design deteriorates. -- 4.1 Add max-width on the component of 360px 5.0 Accessibility -- Keyboard tab order is pretty obvious -- Visually hidden label on the component: “Page number ##, of 6 pages”. -- Use “aria-hidden” to hide the text label from the screen reader.

Implementation Notes:

silvalaura commented 1 year ago

Testing notes re: @ccedrone

1.0 Component has two buttons that navigate to the Previous Page and Next Page. -- 1.1 The Previous Page button is disabled when you're on the first page. ✅ -- 1.2 The Next Page button is disabled when you're on the last page.✅ -- 1.3 Put tooltips on both buttons with labels "Previous page" and "Next page".✅ 2.0 Display the page you're currently on, as well as choose a specific page, using the Native Select component.✅ 3.0 Display the number of pages using a text label.✅ 4.0 The width of the component is dependent on the container it is inside of. But if the component gets too wide, the integrity of the design deteriorates. 🚫 -- 4.1 Add max-width on the component of 360px ✅ 5.0 Accessibility -- Keyboard tab order is pretty obvious✅ -- Visually hidden label on the component: “Page number ##, of 6 pages”. 🚫 -- Use “aria-hidden” to hide the text label from the screen reader. ✅

Other areas tested:

Feedback:

  1. The width of the component does not grow or shrink based on its parent container: image vs image

  2. It may just be me having trouble testing with VoiceOver, but I don't hear the screenreader read "Go to Page #", so I don't know how easy it is to understand that this is a pagination? Also, the visually hidden label says 1 of 6 pages instead of Page number ##, of 6 pages

  3. It's currently not possible to add custom styles. We should also add {...other} to the <SimplePagination> component (it's supported in classic)

  4. We should consider adding testIds to the previous and next buttons. In the case there are ever multiple paginations on a page, we want these to be unique

  5. When the Simple Pagination example is opened on Code Sandbox, there's an error that page prop is missing. We make that prop mandatory for Uncontrolled Paginations, and optional for Controlled Paginations. Also, there is no description on what that prop is and we need to add it: image

  6. The Handle Page Change example does not work for Simple Pagination. The count ends up getting passed instead of the current page. image

  7. Not something changed in this ticket, but we should fix it while we are there: PageButtonSize needs to be exported in index.ts

  8. Focus: when the previous or next button are clicked, the focus disappears. In the classic version, clicking or tapping on previous/next maintains the focus on that arrow.

Here's the codesandbox I used to test: https://codesandbox.io/s/simplepagination-testing-tfwktw

silvalaura commented 1 year ago

Testing notes, pt 2

1.0 Component has two buttons that navigate to the Previous Page and Next Page. -- 1.1 The Previous Page button is disabled when you're on the first page. ✅ -- 1.2 The Next Page button is disabled when you're on the last page.✅ -- 1.3 Put tooltips on both buttons with labels "Previous page" and "Next page".✅ 2.0 Display the page you're currently on, as well as choose a specific page, using the Native Select component.✅ 3.0 Display the number of pages using a text label.✅ 4.0 The width of the component is dependent on the container it is inside of. But if the component gets too wide, the integrity of the design deteriorates. ✅ -- 4.1 Add max-width on the component of 360px ✅ 5.0 Accessibility -- Keyboard tab order is pretty obvious✅ -- Visually hidden label on the component: “Page number ##, of 6 pages”. ✅ -- Use “aria-hidden” to hide the text label from the screen reader. ✅

Other areas tested:

  1. Docs look good ✅
  2. Code sandbox example ✅
  3. Storybook looks good ✅
  4. The width of the component grows or shrinks based on its parent container ✅
  5. It may just be me having trouble testing with VoiceOver, but I don't hear the screenreader read "Go to Page #", so I don't know how easy it is to understand that this is a pagination? 🚫 This may just be me, if you can confirm this works I'm good with it
  6. Also, the visually hidden label says 1 of 6 pages instead of Page number ##, of 6 pages ✅
  7. It's currently not possible to add custom styles. We should also add {...other} to the component (it's supported in classic) ✅
  8. We should consider adding testIds to the previous and next buttons. In the case there are ever multiple paginations on a page, we want these to be unique ✅
  9. When the Simple Pagination example is opened on Code Sandbox, there's an error that page prop is missing. We make that prop mandatory for Uncontrolled Paginations, and optional for Controlled Paginations. Also, there is no description on what that prop is and we need to add it ✅
  10. The Handle Page Change example does not work for Simple Pagination. The count ends up getting passed instead of the current page. ✅
  11. Not something changed in this ticket, but we should fix it while we are there: PageButtonSize needs to be exported in index.ts ✅
  12. Focus: when the previous or next button are clicked, the focus disappears. In the classic version, clicking or tapping on previous/next maintains the focus on that arrow. ✅

Feedback:

  1. Tooltips need the isInverse prop Image
  2. Would love @orion-cengage 's feedback on the spacing: image

    vs.

image

I also noticed that on the min-width of 260px there's an overlap with the hover color: image

CodeSandbox: https://codesandbox.io/s/simple-pagination-test-2-wk2bjm

orion-cengage commented 1 year ago

Thanks for catching this @silvalaura . Yes, the spacing is supposed to be 16px, not 14px. 14px isn't part of our spacing system.

orion-cengage commented 1 year ago

I'm less worried about the overlapping at 260px. That size was not a hard requirement, but simply the smallest I was able to get the component down to in Figma. At some point, the container is too small for the component, and it looks like when it is actually rendered in a browser, 260px is a bit too small.

silvalaura commented 1 year ago

Testing Notes, pt 3

  1. VoiceOver reads the page change perfectly ✔
  2. Spacing is updated to 16px ✔
  3. Tooltip receives the isInverse prop ✔

Closing as done! 💃