-
Notifications
You must be signed in to change notification settings - Fork 128
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
[Bug] Added ResizeObserver
to update VA payments pagination on zoomed or mobile screens
#34245
base: main
Are you sure you want to change the base?
Conversation
* First pass with RO working, no defensive checks. * Added feature check for ResizeObserver.
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 clarifying comments for code optimization and how some defaults came to be.
@@ -174,3 +174,31 @@ export const alertMessage = ( | |||
</p> | |||
</va-alert> | |||
); | |||
|
|||
export const useResizeObserver = callbackFn => { |
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 opted to split the ResizeObserver
into a custom hook to take advantage of useCallback()
caching and prevent unnecessary re-renders. If there's value in raising this up to a higher "helpers" folder, LMK.
() => { | ||
const element = ref?.current; | ||
|
||
if (!window.ResizeObserver && element) return; |
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.
Went ahead and ran a window.ResizeObserver
check even though MDN shows wide availability. Figured it couldn't hurt to be defensive for older browsers or devices.
|
||
const MAX_PAGE_LIST_LENGTH = 10; | ||
const MAX_PAGES_CONTAINER_WIDTH = 640; // USWDS width-tablet setting |
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.
In the original issue, we started seeing issues when the container width fell below ~500 pixels with 10 pagination links. I didn't want to have a random magic number in the code, so I fell back on the USWDS breakpoint that was closest to this number: https://designsystem.digital.gov/utilities/height-and-width/
Are you removing, renaming or moving a folder in this PR?
Summary
ResizeObserver
was added to the Your VA Payments table pagination to ensure pagination links do not exceed the width of their container.ResizeObserver
to update the number of pagination links when the container width falls below 640px. This provides a better experience for users on mobile devices or who prefer to zoom their layouts in to 200, 300, or 400%.Related issue(s)
Testing done (TBD)
Screenshots (TBD)
Note: This field is mandatory for UI changes (non-component work should NOT have screenshots).
What areas of the site does it impact?
Code is localized to the
Your VA Payments
view.Acceptance criteria
Quality Assurance & Testing
ResizeObserver
fired correctlyDocumentation has been updated (link to documentation *if necessary)Error Handling
Events are being sent to the appropriate logging solutionFeature/bug has a monitor built into Datadog or Grafana (if applicable)Authentication
Requested Feedback
(OPTIONAL) What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?
I would appreciate reviewers checking this feature locally using their preferred browsers and OS as time permits. This is a case where some users have a lot of payments and the pagination table was too wide for the container.