-
Notifications
You must be signed in to change notification settings - Fork 13
va-pagination: update for maximum of seven slots #1476
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
Conversation
* Updated Storybook instances to five as default maxPageListLength. * Added tests for mobile display and mobile axe check. * Added comment for mobile testing device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments to help clarify some of my refactoring decisions.
{this.renderPreviousButton(arrowClasses, previousAriaLabel)} | ||
{this.renderFirstPage(itemClasses, ellipsisClasses)} | ||
{this.renderMiddlePages(ariaLabelSuffix, itemClasses)} | ||
{this.renderLastPage(ellipsisClasses, itemClasses)} | ||
{this.renderNextPageButton(arrowClasses, nextAriaLabel)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-architected these into private functions so we could separate the business logic from the render()
method. I felt it made the "view layer" easier to read and co-located each piece of the pagination. If there's a better way to render these than individual functions, LMK!
// The smallest breakpoint can show at most 6 pages, including ellipses | ||
if (isMobileViewport) maxPageListLengthState = SHOW_ALL_PAGES; | ||
|
||
// Use cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lightly refactored the pageNumbers()
function to use if, else if, else
logic. I explored using a switch case but didn't feel it was going to scale with multiple props needed to derive the final arrays.
previousAriaLabel: string, | ||
): JSX.Element { | ||
// Skip rendering if the viewport is too narrow to show Previous button | ||
if (this.isMobileViewport) return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I flipped the logic on these new UI render functions. If a condition exists that means this piece of pagination should not be rendered, I returned early using guard clauses. This helped me focus the return statements on the exact markup we wanted.
I tested the change with NVDA, JAWS, and ZoomText on Win11, using Chrome, Firefox, and Edge. Also did basic testing with MS Voice Access to confirm Next, Previous, page numbers clicked as expected. |
va-pagination
to adapt to mobile devices and zoomed layouts
packages/web-components/src/components/va-pagination/va-pagination.tsx
Outdated
Show resolved
Hide resolved
@@ -55,3 +56,9 @@ button { | |||
transition: all 0.3s ease-in-out; | |||
transition-property: color, background-color; | |||
} | |||
|
|||
@container (max-width: 640px) { // Variables won't compile. Matching TSX file for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Variables won't compile. Matching TSX file for now.
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was having quite a time passing the USWDS $tablet
variable into this container query. I tried var(--tablet)
and $tablet
and #${tablet}
but they either wouldn't render the CSS rule at all, or failed during compilation. Happy to change it if there's something I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really interesting 🤔 I wonder if that's a Stencil limitation? Thanks for explaining!
Do you mind just mentioning in the comment that the value is for the --tablet
variable/property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a limitation in Sass. I found this article Want CSS variables in media query declarations? Try this! that digs into the specifics of Sass variables in media and container queries. I'll update the comment explaining the issue.
/** | ||
* ========================================= | ||
* End pagination render functions | ||
* ========================================= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'm personally not a fan of adding these large comment blocks as waypoints for code sections (here and on line 301). Function names should already be identifying what they're for and it seems like they do. I don't think the design system team has code style guidelines yet but I don't see these types of comment blocks in any other components either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem, I'll take them out in just a short.
…tion.tsx Co-authored-by: Andrew Steele <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you double check the unbounded states behavior? If do the following steps for this unbounded middle example, it seems like I can get to a point where I can't click further than page 5. It doesn't seem like the current Storybook example does that.
- Go to unbounded middle example
- Click 1
- Click 5 and then 6
Yes, that looks like a hole in the new logic I introduced. I'll look into it later this afternoon and into tomorrow morning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@1Copenut Thank you so much for this massive effort! I left a few super minor comments and all of the testing looks pretty good on my end too except one thing I want to check in about.
The USWDS behavior guidelines says:
The component features a maximum of seven slots.
But it looks like now there are more than that. Would that be fairly straight forward to get back in alignment in this PR?
* The number has been reduced from 7 to 6 to show all more consistently | ||
* on small screens and zoomed in browser windows. | ||
*/ | ||
SHOW_ALL_PAGES: number = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this comment make sense in the long run? Should this be reverted back to the original comment?
If the page total is less than or equal to this limit, show all pages.
|
||
/** | ||
* Mobile viewport width chosen based on USWDS "mobile-lg" spacing unit. | ||
* See https://designsystem.digital.gov/design-tokens/spacing-units/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would prefer this to reference VADS which also uses the same named/value mobile-lg
token: https://design.va.gov/foundation/breakpoints
This can be updated in a few different places.
|
||
private handlePageSelect = (page, eventID) => { | ||
/** | ||
* Tabllet viewport width chosen based on USWDS "tablet" spacing unit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Tabllet
return makeArray(1, totalPages); | ||
} | ||
} else if (SHOW_ALL_PAGES < totalPages && totalPages <= maxPageListLength) { | ||
console.log('Use case 2'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup console.logs in a few different places
@jamigibbs I appreciate the quick review and comments, thank you. I'll remove the errant console statements and mis-spellings in a quick push and focus on getting the total count back in line with production examples (7 or fewer). Will ping you after I have those updates in place. |
Much simpler API now. Removed two props that appeared to be legacy from V1. All pagination layouts now assume there will be a maximum of 7 slots on the screen at any one time. The only hitch is at the smallest breakpoint with 7 pages exactly, the first and last pagination links break out of the container and disappear. I have a I'm testing locally in |
Additional context regarding the seven slot max update in this Slack convo: https://dsva.slack.com/archives/C01DBGX4P45/p1741301043783969 |
Thank you Jami. Also, this is a local implementation in VA Payment History. Our container is ~260px wide when zoomed in to 400% and the pagination overflows the container. This could be solved locally with app-specific CSS, but will need to be solved to fully meet WCAG SC 1.4.10 Reflow (Level AA). |
Hey @1Copenut - The not fitting on mobile is an issue that we have a few issues for already. See #3003 and #2597. Because this issue stems from the USWDS component, we also filed an issue in their repo too. It looks like they may pick it up soon. It's a deceptively tricky problem considering accessibility guidelines on touch target size, spacing between page links, showing next/previous links, etc in such a small space. |
Thanks, @1Copenut! That update is probably what I would have done too since it fixes the layout at 320px for mobile when there's overflow (i.e. ellipses). Just a heads up, if there are exactly 7 pages — so no ellipses — the page buttons will still get cut off at 320px. (Granted, this is likely a rare occurrence.) @department-of-veterans-affairs/platform-design-system-fe: Just want to note that the USWDS has the issue we submitted for this in the their most recent sprint "2025.3 Feb 17 - Mar 9". So code updates from that may be coming our way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great in my testing @1Copenut! I left just a few comments and it's mostly just cleanup requests. Thanks again for your persistence. 💪
* See https://dev.to/pubnub/how-fast-is-real-time-human-perception-and-technology-1308 | ||
* for how the 100ms default was selected. | ||
*/ | ||
export function debounce(callbackFn: any, delay: number = 100) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this utility function isn't needed anymore, we can probably remove it.
@@ -80,21 +75,20 @@ export class VaPagination { | |||
* Display last page number when the page count exceeds | |||
* `maxPageListLength` | |||
*/ | |||
@Prop() showLastPage?: boolean = false; | |||
// @Prop() showLastPage?: boolean = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this prop also being removed? If so, can you delete instead of commenting it out?
Also make sure to run the yarn build
step again so that the Stencil generated file gets updated.
@@ -119,72 +113,67 @@ export class VaPagination { | |||
*/ | |||
private pageNumbers = () => { | |||
const { | |||
maxPageListLength, | |||
// maxPageListLength, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleanup
if (totalPages <= this.SHOW_ALL_PAGES) { | ||
if (totalPages <= SHOW_ALL_PAGES) { | ||
// Use case #1: 7 or fewer total pages | ||
console.log('Use case 1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove console.logs throughout 🙏🏻
|
||
@container (max-width: 480px) { | ||
// This value matches the USWDS --mobile-lg custom property | ||
// See https://designsystem.digital.gov/design-tokens/spacing-units/#spacing-unit-tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we link to the VADS documentation instead in this comment? https://design.va.gov/foundation/breakpoints
@@ -100,13 +100,13 @@ describe('va-pagination', () => { | |||
expect(pageNumbers[pageNumbers.length - 1].innerHTML).toEqual('6'); | |||
}); | |||
|
|||
it('renders all page numbers if total pages is less than or equal to 7', async () => { | |||
it('renders all page numbers if total pages is less than or equal to 6', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a test for this already but can you double check if there's one that validates it doesn't exceed 7 slots?
const page = await newE2EPage(); | ||
await page.setViewport({ | ||
// iPhone X | ||
width: 375, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like these setViewport
tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😮 Wow, this is awesome! Can we add or update mobile to be 320px and still run these tests? That's a iPhone SE (1st gen) resolution and the size we currently design mobile assets for.
@jamigibbs Ready for another review. I updated and cleaned up files as requested. Added a couple of tests for first and last page logic testing too. |
Chromatic
https://tpierce-2199-bug-va-pagination--65a6e2ed2314f7b8f98609d8.chromatic.com
Configuring this pull request
major
,minor
,patch
, orignore-for-release
).ignore-for-release
if files haven't been changed in a component library package. (ie. only Storybook files)/packages/core
version number if this will be the last PR merged before a release.Description
Closes department-of-veterans-affairs/vets-design-system-documentation#2199.
This PR adds a
resizeObserver
to theva-pagination
component to update the number of pagination links on smaller device viewports and zoomed browser windows.QA Checklist
Screenshots
These are screenshots from a local test on
vets-website
having published the components on Verdaccio (local registry).Small screen

Medium screen

Large screen

Acceptance criteria
Definition of done