Skip to content
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

[Enhancement] Add useMemo and useCallback to useSnackbar Hook #617

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

oddballdave
Copy link

@oddballdave oddballdave commented Jan 7, 2025

Description of Change

Added memoization to the useSnackbar hook by utilizing useCallback and useMemo. This prevents the snackbar from being re-rendered when utilized inside a useEffect hook by the caller, which could have the undesirable effect of leaving the snackbar visible when it was intended to be hidden.

Testing Packages

Screenshots/Video

These before/after videos show the issue in the VA Mobile app. Notice how in the 'Before' video the snackbar does not correctly get dismissed when backing up a screen.

Snackbar.Before.mp4
Snackbar.After.mp4

Testing

Testing was performed in the VA Mobile app using this branch which has yet to be deployed. In order to force the Snackbar to appear, the downloadDecisionLetter function was temporarily modified to throw an exception. After clicking on a claim letter, the Snackbar will appear. When navigating back a screen the Snackbar should disappear, but it does not without the memoization fix in this PR.

  • Tested on iOS
  • Tested on Android
  • Tested on Web

PR Checklist

Code reviewer validation:

  • General
    • PR is linked to ticket(s)
    • PR has changelog label applied if it's to be included in the changelog
    • Acceptance criteria:
      • All satisfied or
      • Documented reason for not being performed or
      • Split to separate ticket and ticket is linked by relevant AC(s)
    • Above PR sections adequately filled out
    • If any breaking changes, in accordance with the pre-1.0.0 versioning guidelines: a CU ticket has been created for the VA Mobile App detailing necessary adjustments with the package version that will be published by this ticket
  • Code
    • Tests are included if appropriate (or split to separate ticket)
    • New functions have proper TSDoc annotations

Publish

If changes warrant a new version per the versioning guidelines and the PR is approved and ready to merge:

@oddballdave oddballdave linked an issue Jan 7, 2025 that may be closed by this pull request
3 tasks
@oddballdave oddballdave changed the title [Enhancement] Memoize Snackbar Component [Enhancement] Add useMemo and useCallback to useSnackbar Hook Jan 8, 2025
@oddballdave oddballdave marked this pull request as ready for review January 8, 2025 16:00
@oddballdave oddballdave requested a review from a team as a code owner January 8, 2025 16:00
@oddballdave oddballdave marked this pull request as draft January 14, 2025 19:45
@oddballdave
Copy link
Author

Reverting to draft until #621 is approved.

Copy link

@DonMcCaugheyUSDS DonMcCaugheyUSDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me (caveat: I'm not an expert on React). Thanks for the details on how you tested this.

Let's ship it.

@Eallan919
Copy link

These PR's are blocked because we are not authorized to merge PR's to main in the DS Library: #617 & #621. We are also likely blocked on publishing/releasing the DS Library package, which happens after we merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DS - Add useMemo and useCallback to useSnackbar hook
4 participants