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.

Sign up for GitHub

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

Jump to bottom

Chore/9630-UseDesignSystemSnackbarComponent #10298

Open
wants to merge 36 commits into
base: develop
Choose a base branch
from
Open

Chore/9630-UseDesignSystemSnackbarComponent #10298

wants to merge 36 commits into from
+393 −845

Conversation

oddballdave
Copy link
Contributor

@oddballdave oddballdave commented Dec 3, 2024 •
edited by timwright12
Loading

Description of Change

  • Completely removed the old global SnackBar implementation and replaced it with the design system Snackbar component
  • Incorporated the use of the Snackbar provider and hook from the design system
  • Patched the design system Snackbar to workaround dependency issues with the useEffect hook
  • Implemented automatic dismissal of the Snackbar when the user navigates away from the current screen
  • Removed SnackbarMessages and switched to using translated text directly
  • Modified unit tests to work correctly with the new Snackbar implementation
  • Cleaned up unused files, constants, string resources and related portions of the app-level theme

Screenshots/Video

Android (Before)
Android (After)
iOS (Before)
iOS (After)

Testing

  • Tested on iOS
  • Tested on Android

Reviewer Validations

PR Checklist

Reviewer: Confirm the items below as you review

  • PR is connected to issue(s)
  • Tests are included to cover this change (when possible)
  • No magic strings (All string unions follow the Union -> Constant type pattern)
  • No secrets or API keys are checked in
  • All imports are absolute (no relative imports)
  • New functions and Redux work have proper TSDoc annotations

For QA

Run a build for this branch

@oddballdave oddballdave linked an issue Dec 3, 2024 that may be closed by this pull request
Global SnackBar Implementation #9630
Open
@oddballdave oddballdave marked this pull request as ready for review December 4, 2024 13:33
@oddballdave oddballdave requested review from a team as code owners December 4, 2024 13:33
Sparowhawk
Sparowhawk previously approved these changes Dec 4, 2024
View reviewed changes
@timwright12
Copy link
Contributor

@oddballdave please make sure to not merge until QA has tested and approved. This is a large change, so it will take some time.

narin
narin suggested changes Dec 6, 2024
View reviewed changes
Copy link
Contributor

@narin narin left a comment

Choose a reason for hiding this comment

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

Looks great for the most part. We'll consider updating the component library to use the patch you created.

Reiterating what we discussed on the call, for Snackbars that need to appear on screens using templates without the nav bar showing such as <FullScreenSubtask /> or <LargePanel />, if a Snackbar is shown and the user isn't immediately navigated back to a screen that has the nav bar showing, the offset option should be included with snackbar.show() to account for the lack of nav bar. This probably would occur mostly with error message Snackbars. You can use theme.dimensions.snackBarBottomOffset as the offset which automatically calculates the correct offset based on OS.

i.e.

snackbar.show(t('secureMessaging.startNewMessage.sent.error')'), { offset: theme.dimensions.snackBarBottomOffset })

@TimRoe TimRoe mentioned this pull request Dec 12, 2024
Open
2 tasks
TimRoe
TimRoe previously approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@TimRoe TimRoe left a comment

Choose a reason for hiding this comment

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

Approving for my part (still needs app team approval) based on the changes.

TimRoe
TimRoe previously approved these changes Dec 16, 2024
View reviewed changes
@oddballdave oddballdave marked this pull request as draft January 7, 2025 15:47
@oddballdave
Copy link
Contributor Author

Moved back to draft until this DS library PR can be incorporated.

@Eallan919
Copy link

These PR's are blocked because they depend on updating the DS Library with the previously mentioned PR's and they also need to have a final review by an authorized user: #10298 & #10392.

@Eallan919 Eallan919 mentioned this pull request Jan 27, 2025
Open
8 tasks
@oddballdave oddballdave linked an issue Feb 25, 2025 that may be closed by this pull request
BUG - sev-3 - Android - TalkBack: Unable to use tap to focus on snackbar if other focusable content is behind snackbar #5055
Open
4 tasks
@oddballdave oddballdave marked this pull request as ready for review February 26, 2025 13:51
matt-guest-wilcore
matt-guest-wilcore reviewed Feb 26, 2025
View reviewed changes
...n/ClaimDetailsScreen/ClaimStatus/ClaimFileUpload/AskForClaimDecision/AskForClaimDecision.tsx
},
onError: () => showSnackBar('Request could not be sent', dispatch, () => onSubmit, false, true),
onError: () =>
snackbar.show('Request could not be sent', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we localize 'Request could not be sent'? I see all the other snackbar messages are using localized strings.

matt-guest-wilcore
matt-guest-wilcore reviewed Feb 26, 2025
View reviewed changes
...n/ClaimDetailsScreen/ClaimStatus/ClaimFileUpload/AskForClaimDecision/AskForClaimDecision.tsx
@@ -102,9 +102,14 @@ function AskForClaimDecision({ navigation, route }: AskForClaimDecisionProps) {
const mutateOptions = {
onSuccess: () => {
setSubmittedDecision(true)
showSnackBar('Request sent', dispatch, undefined, true, false, true)
snackbar.show('Request sent')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the comment below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matt-guest-wilcore matt-guest-wilcore matt-guest-wilcore left review comments

@TimRoe TimRoe TimRoe left review comments

@Sparowhawk Sparowhawk Sparowhawk left review comments

@timwright12 timwright12 Awaiting requested review from timwright12 timwright12 is a code owner automatically assigned from department-of-veterans-affairs/flagship-mobile-reviewers

@alexandec alexandec Awaiting requested review from alexandec alexandec is a code owner automatically assigned from department-of-veterans-affairs/flagship-mobile-reviewers

@theodur theodur Awaiting requested review from theodur theodur is a code owner automatically assigned from department-of-veterans-affairs/flagship-mobile-reviewers

@dumathane dumathane Awaiting requested review from dumathane dumathane is a code owner automatically assigned from department-of-veterans-affairs/flagship-mobile-reviewers

@cadibemma cadibemma Awaiting requested review from cadibemma cadibemma is a code owner automatically assigned from department-of-veterans-affairs/flagship-mobile-reviewers

@DonMcCaugheyUSDS DonMcCaugheyUSDS Awaiting requested review from DonMcCaugheyUSDS DonMcCaugheyUSDS is a code owner automatically assigned from department-of-veterans-affairs/flagship-mobile-reviewers

@narin narin Awaiting requested review from narin

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@oddballdave @timwright12 @Eallan919 @narin @TimRoe @Sparowhawk @matt-guest-wilcore @BeccaTupaj