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

Chore/9630-UseDesignSystemSnackbarComponent #10298

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
3f5c125
Incorporated SnackbarProvider
oddballdave Nov 20, 2024
4289723
Incorporated Design System Snackbar Component into all Benefits screens
oddballdave Nov 21, 2024
8ce113a
Incorporated Design System Snackbar Component into all Health screens
oddballdave Nov 21, 2024
7a1fa17
Incorporated Design System Snackbar Component into all Home screens
oddballdave Nov 22, 2024
dbd2db7
Incorporated Design System Snackbar Component into all Payments screens
oddballdave Nov 22, 2024
487ff8d
Removed global snackBar and other related references
oddballdave Nov 22, 2024
6439410
Removed SnackBar component, snackBarSlice and related references
oddballdave Dec 2, 2024
5e25f8d
Patched Design System Snackbar to memoize useSnackbar
oddballdave Dec 2, 2024
4d4e0f6
Merge branch 'develop' into chore/9630-UseDesignSystemSnackbarComponent
oddballdave Dec 3, 2024
1940541
Pod and project updates
oddballdave Dec 3, 2024
9228e5d
Removed unused import
oddballdave Dec 3, 2024
81cb504
Merge branch 'develop' into chore/9630-UseDesignSystemSnackbarComponent
oddballdave Dec 4, 2024
8f660fb
Merge branch 'develop' into chore/9630-UseDesignSystemSnackbarComponent
oddballdave Dec 4, 2024
ff4da29
Add back screenListeners to hide the snackbar when it's not handled i…
oddballdave Dec 6, 2024
1c15325
Adjusted NAV_BAR_HEIGHT to its actual value and added marginBottom to…
oddballdave Dec 10, 2024
ad853bb
Added screenListeners to hide snackbar when navigating
oddballdave Dec 10, 2024
6b3d569
Set snackbar offset to 0 for screens where there is no bottom navigat…
oddballdave Dec 10, 2024
a6013e9
Removed unused vama_snackbar_null
oddballdave Dec 10, 2024
ec3c641
Merge branch 'develop' into chore/9630-UseDesignSystemSnackbarComponent
oddballdave Dec 10, 2024
0580fb5
Reverted patch changes to adjust NAV_BAR_HEIGHT and marginBottom
oddballdave Dec 12, 2024
c36561f
Added snackBarBottomOffset back to the theme and started using it as …
oddballdave Dec 12, 2024
c830268
Merge branch 'develop' into chore/9630-UseDesignSystemSnackbarComponent
oddballdave Dec 12, 2024
d1626ec
Fixed linter warning
oddballdave Dec 12, 2024
63b4ec8
Merge branch 'develop' into chore/9630-UseDesignSystemSnackbarComponent
oddballdave Dec 12, 2024
b9d4b2c
Merge branch 'develop' into chore/9630-UseDesignSystemSnackbarComponent
oddballdave Dec 14, 2024
d0c2f9a
Modifications to use new design system snackbar
oddballdave Dec 14, 2024
7d8568f
Merge branch 'develop' into chore/9630-UseDesignSystemSnackbarComponent
oddballdave Dec 16, 2024
ee7371d
Merge branch 'develop' into chore/9630-UseDesignSystemSnackbarComponent
oddballdave Dec 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
diff --git a/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/Snackbar.tsx b/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/Snackbar.tsx
index 6b2f54d..8931ba9 100644
--- a/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/Snackbar.tsx
+++ b/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/Snackbar.tsx
@@ -160,6 +160,7 @@ export const Snackbar: FC<SnackbarProps> = (toast) => {
minHeight: 44,
padding: spacing.vadsSpaceSm,
marginHorizontal: spacing.vadsSpaceLg,
+ marginBottom: spacing.vadsSpaceLg,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the justification for this in another comment:

I made another patch to the Snackbar component to update the navigation bar height (more accurate) and to add a bottom margin to the Snackbar. This seemed like the right place to fix things since we are already accounting for the left/right margins and navigation bar height in the component itself versus relying on the app to provide these properties.

The reason bottom margin was not included in the component is two fold:

  1. It's controllable via the offset to whatever an app could want so we just gave a default as apps would need to override it anyway for any special cases--if 20 pixel spacing is desired without a Nav bar, offset can be set to 20
  2. Margin makes elements underneath the Snackbar (e.g. a button sticking out partially) no longer pressable within the margined space despite being visible where setting the offset via the library used by the component allows presses outside the bounds of the Snackbar to still occur as desired
    • For full disclosure: when building it, I tried to use margin instead of the library offset functionality since it was simpler, but found it resulted in the bug where you could position it above the Nav bar visually, but none of the Nav bar buttons would be able to be pressed; using the offset functionality, the Nav bar can still be interacted with as meets user expectations

The side margins were deemed acceptable to block presses as, paired with general padding for content, anything peeking should be very minimal.

I technically defer to the app team since we on the design system team long term are not responsible for how apps implement our components, but I'd recommend against patching this in as patches are frail and this is liable to break from future library changes we make to the component when updating while leveraging the offset should not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. Just so we're all on the same page, the left screenshot below is the default location where the Snackbar gets positioned when a bottom navigation bar is present and no offset is specified. There are 4 units of vertical spacing between the top of the navbar and the Snackbar. The right screenshot shows the changes with the patch applied (20 units of vertical spacing). If we can live with the spacing on the left screenshot, then I can remove the patch easily enough. If we want more spacing but no patch, then the app will have to start passing offsets into all Snackbars. Please advise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the 4 units spacing is fine, but ultimately seems like a design decision. I believe the position is what it was with the old Snackbar so it'd be maintaining existing functionality and didn't come up during creating it or we'd likely have gone with a different default. As a "floating" element it can appear directly above other content regardless if it's 4 or 20 pixels from the Nav bar--the crux is not blocking the Nav bar so it's usable.

If the increased spacing is determined the way: I think a patch is fair way to go about it, but it would be better to update the default offset to 76 instead of using marginBottom. Longer term, we have done research into global overrides for certain design system parameters and the Snackbar offset definitely would be a good use case--but, in the absence of that yet existing, patching in a different default value seems like a reasonable workaround since when the global override exists then the only code change would be removing the patch and setting the global override in one spot.

Not sure if @Sparowhawk or anyone else on the app side had thoughts since ultimately from the design system side we're just providing recommendations based on how the components were built/designed and consumer apps can do what they want.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a circle back: created a ticket to add a global override for the default Snackbar offset so it's documented as a desired future enhancement.

rowGap: spacing.vadsSpace2xs,
},
}
diff --git a/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/useSnackbar.tsx b/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/useSnackbar.tsx
index 2e5c464..a696fa9 100644
--- a/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/useSnackbar.tsx
Expand Down Expand Up @@ -41,3 +53,15 @@ index 2e5c464..a696fa9 100644
+ }), [show, toast.hideAll, toast.isOpen]
+ )
}
diff --git a/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/useSnackbarDefaultOffset.tsx b/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/useSnackbarDefaultOffset.tsx
index e7679fa..135c5fa 100644
--- a/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/useSnackbarDefaultOffset.tsx
+++ b/node_modules/@department-of-veterans-affairs/mobile-component-library/src/components/Snackbar/useSnackbarDefaultOffset.tsx
@@ -1,6 +1,6 @@
import { useSafeAreaInsets } from 'react-native-safe-area-context'

-const NAV_BAR_HEIGHT = 60
+const NAV_BAR_HEIGHT = 56
Copy link
Contributor

Choose a reason for hiding this comment

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

This is somewhat a design decision, but am not sure pixel perfection is necessary here.

Similar to the above of deferring to the app team, but I'd recommend against patching it vs having 4 pixels more in what is already somewhat arbitrary padding for a "floating" element.


/**
* Returns default Snackbar offset depending on safe area bottom inset
17 changes: 15 additions & 2 deletions VAMobile/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { createBottomTabNavigator } from '@react-navigation/bottom-tabs'
import { NavigationContainer, useNavigationContainerRef } from '@react-navigation/native'
import { createStackNavigator } from '@react-navigation/stack'

import { SnackbarProvider } from '@department-of-veterans-affairs/mobile-component-library'
import { SnackbarProvider, useSnackbar } from '@department-of-veterans-affairs/mobile-component-library'
import { ActionSheetProvider, connectActionSheet } from '@expo/react-native-action-sheet'
import { QueryClientProvider } from '@tanstack/react-query'
import { ThemeProvider } from 'styled-components'
Expand Down Expand Up @@ -406,6 +406,7 @@ export function AppTabs() {
}

export function AuthedApp() {
const snackbar = useSnackbar()
const headerStyles = useHeaderStyles()
const { initialUrl } = useNotificationContext()
const homeScreens = getHomeScreens()
Expand All @@ -423,7 +424,19 @@ export function AuthedApp() {

return (
<>
<RootNavStack.Navigator screenOptions={{ ...headerStyles, detachPreviousScreen: false }} initialRouteName="Tabs">
<RootNavStack.Navigator
screenOptions={{ ...headerStyles, detachPreviousScreen: false }}
initialRouteName="Tabs"
screenListeners={{
transitionStart: (e) => {
if (e.data.closing) {
snackbar.hide()
}
},
blur: () => {
snackbar.hide()
},
}}>
<RootNavStack.Screen
name="Tabs"
component={AppTabs}
Expand Down
18 changes: 17 additions & 1 deletion VAMobile/src/components/Templates/MultiStepSubtask.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import React, { createContext, useCallback, useContext, useState } from 'react'
import { ParamListBase, useFocusEffect } from '@react-navigation/native'
import { TransitionPresets, createStackNavigator } from '@react-navigation/stack'

import { useSnackbar } from '@department-of-veterans-affairs/mobile-component-library'

import { FullScreenSubtask, FullScreenSubtaskProps } from 'components'

/**
Expand Down Expand Up @@ -38,13 +40,27 @@ type MultiStepSubtaskProps<T extends ParamListBase> = FullScreenSubtaskProps & {
}

function MultiStepSubtask<T extends ParamListBase>({ ...props }: MultiStepSubtaskProps<T>) {
const snackbar = useSnackbar()
const { children, stackNavigator } = props
const [subtaskProps, setSubtaskProps] = useState<FullScreenSubtaskProps>({})

return (
<FullScreenSubtask {...subtaskProps}>
<SubtaskContext.Provider value={{ setSubtaskProps }}>
<stackNavigator.Navigator screenOptions={subtaskScreenOptions}>{children}</stackNavigator.Navigator>
<stackNavigator.Navigator
screenOptions={subtaskScreenOptions}
screenListeners={{
transitionStart: (e) => {
if (e.data.closing) {
snackbar.hide()
}
},
blur: () => {
snackbar.hide()
},
}}>
{children}
</stackNavigator.Navigator>
</SubtaskContext.Provider>
</FullScreenSubtask>
)
Expand Down
8 changes: 0 additions & 8 deletions VAMobile/src/constants/analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1149,14 +1149,6 @@ export const Events = {
name: 'vama_sm_start',
}
},
// vama_snackbar_null: (location: string): Event => {
// return {
// name: 'vama_snackbar_null',
// params: {
// location,
// },
// }
// },
vama_sso_cookie_received: (received: boolean): Event => {
return {
name: 'vama_sso_cookie_received',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ function AskForClaimDecision({ navigation, route }: AskForClaimDecisionProps) {
setSubmittedDecision(true)
snackbar.show('Request sent')
},
onError: () => snackbar.show('Request could not be sent', { isError: true, onActionPressed: () => onSubmit }),
onError: () =>
snackbar.show('Request could not be sent', { isError: true, offset: 0, onActionPressed: () => onSubmit }),
}
submitClaimDecision(claimID, mutateOptions)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,11 @@ function UploadFile({ navigation, route }: UploadFileProps) {
snackbar.show(t('fileUpload.submitted'))
},
onError: () =>
snackbar.show(t('fileUpload.submitted.error'), { isError: true, onActionPressed: onUploadConfirmed }),
snackbar.show(t('fileUpload.submitted.error'), {
isError: true,
offset: 0,
onActionPressed: onUploadConfirmed,
}),
}
const params: UploadFileToClaimParamaters = { claimID, documentType: documentType, request, files: filesList }
uploadFileToClaim(params, mutateOptions)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,11 @@ function UploadOrAddPhotos({ navigation, route }: UploadOrAddPhotosProps) {
snackbar.show(t('fileUpload.submitted'))
},
onError: () =>
snackbar.show(t('fileUpload.submitted.error'), { isError: true, onActionPressed: onUploadConfirmed }),
snackbar.show(t('fileUpload.submitted.error'), {
isError: true,
offset: 0,
onActionPressed: onUploadConfirmed,
}),
}
const params: UploadFileToClaimParamaters = {
claimID,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export function useComposeCancelConfirmation(): [
onError: () => {
snackbar.show(t('secureMessaging.draft.saved.error'), {
isError: true,
offset: 0,
onActionPressed: () => saveDraft(params, mutateOptions),
})
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ function EditDraft({ navigation, route }: EditDraftProps) {
const params: SendMessageParameters = { messageData: messageData, uploads: attachmentsList }
snackbar.show(t('secureMessaging.startNewMessage.sent.error'), {
isError: true,
offset: 0,
onActionPressed: () => sendMessage(params, mutateOptions),
})
}
Expand Down Expand Up @@ -271,6 +272,7 @@ function EditDraft({ navigation, route }: EditDraftProps) {
onError: () => {
snackbar.show(t('secureMessaging.deleteDraft.snackBarErrorMessage'), {
isError: true,
offset: 0,
onActionPressed: () => deleteDraft(params, mutateOptions),
})
},
Expand Down Expand Up @@ -478,6 +480,7 @@ function EditDraft({ navigation, route }: EditDraftProps) {
onError: () => {
snackbar.show(t('secureMessaging.draft.saved.error'), {
isError: true,
offset: 0,
onActionPressed: () => saveDraft(params, mutateOptions),
})
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ function ReplyMessage({ navigation, route }: ReplyMessageProps) {
onError: () => {
snackbar.show(t('secureMessaging.draft.saved.error'), {
isError: true,
offset: 0,
onActionPressed: () =>
saveDraft(
{
Expand Down Expand Up @@ -253,6 +254,7 @@ function ReplyMessage({ navigation, route }: ReplyMessageProps) {
} else {
snackbar.show(t('secureMessaging.startNewMessage.sent.error'), {
isError: true,
offset: 0,
onActionPressed: () =>
sendMessage(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ function StartNewMessage({ navigation, route }: StartNewMessageProps) {
onError: () => {
snackbar.show(t('secureMessaging.draft.saved.error'), {
isError: true,
offset: 0,
onActionPressed: () => {
// passing messageDataRef to ensure we have the latest messageData
saveDraft({ messageData: messageDataRef.current }, mutateOptions)
Expand All @@ -332,6 +333,7 @@ function StartNewMessage({ navigation, route }: StartNewMessageProps) {
} else {
snackbar.show(t('secureMessaging.startNewMessage.sent.error'), {
isError: true,
offset: 0,
onActionPressed: () => {
// passing messageDataRef to ensure we have the latest messageData
sendMessage({ messageData: messageDataRef.current, uploads: attachmentsList }, mutateOptions)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,11 @@ function AddressValidation({
const mutateOptions = {
onSuccess: () => snackbar.show(GenerateAddressMessage(t, addressType, false)),
onError: () =>
snackbar.show(GenerateAddressMessage(t, addressType, true), { isError: true, onActionPressed: () => save }),
snackbar.show(GenerateAddressMessage(t, addressType, true), {
isError: true,
offset: 0,
onActionPressed: () => save,
}),
}
saveAddress({ addressData, revalidate }, mutateOptions)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ function EditAddressScreen({ navigation, route }: IEditAddressScreen) {
onError: () =>
snackbar.show(t('contactInformation.residentialAddress.removed.error'), {
isError: true,
offset: 0,
onActionPressed: () => deleteAddress,
}),
}
Expand Down Expand Up @@ -274,7 +275,11 @@ function EditAddressScreen({ navigation, route }: IEditAddressScreen) {
}
},
onError: () =>
snackbar.show(GenerateAddressMessage(t, addressType, true), { isError: true, onActionPressed: () => save }),
snackbar.show(GenerateAddressMessage(t, addressType, true), {
isError: true,
offset: 0,
onActionPressed: () => save,
}),
},
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,11 @@ function EditEmailScreen({ navigation }: EditEmailScreenProps) {
onError: (error: unknown) => {
if (isErrorObject(error)) {
if (error.status === 400) {
snackbar.show(t('contactInformation.emailAddress.not.saved'), { isError: true })
snackbar.show(t('contactInformation.emailAddress.not.saved'), { isError: true, offset: 0 })
} else {
snackbar.show(t('contactInformation.emailAddress.not.saved'), {
isError: true,
offset: 0,
onActionPressed: () => saveEmail(emailData, mutateOptions),
})
}
Expand Down Expand Up @@ -130,6 +131,7 @@ function EditEmailScreen({ navigation }: EditEmailScreenProps) {
onError: () =>
snackbar.show(t('contactInformation.emailAddress.not.removed'), {
isError: true,
offset: 0,
onActionPressed: () => deleteEmail(emailData, mutateOptions),
}),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ function EditPhoneNumberScreen({ navigation, route }: IEditPhoneNumberScreen) {
isErrorObject(error) &&
snackbar.show(t('contactInformation.phoneNumber.not.saved', { type: displayTitle }), {
isError: true,
offset: 0,
onActionPressed: save,
}),
}
Expand All @@ -147,6 +148,7 @@ function EditPhoneNumberScreen({ navigation, route }: IEditPhoneNumberScreen) {
isErrorObject(error) &&
snackbar.show(t('contactInformation.phoneNumber.not.removed', { type: displayTitle }), {
isError: true,
offset: 0,
onActionPressed: () => deletePhoneNumber(phoneData, mutateOptions),
}),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ function GenderIdentityScreen({ navigation }: GenderIdentityScreenProps) {
onError: () =>
snackbar.show(t('personalInformation.genderIdentity.not.saved'), {
isError: true,
offset: 0,
onActionPressed: updateGenderIdentity,
}),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ function PreferredNameScreen({ navigation }: PreferredNameScreenProps) {
onError: () =>
snackbar.show(t('personalInformation.preferredName.notSaved'), {
isError: true,
offset: 0,
onActionPressed: updatePreferredName,
}),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ function EditDirectDepositScreen({ navigation, route }: EditDirectDepositProps)
if (!routingNumberError) {
snackbar.show(t('directDeposit.saved.error'), {
isError: true,
offset: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Narin had already suggested it somewhere, but also suggesting having the offset for non-Nav screens set in the app theming instead of having so many hard coded 0's. Make it much easier to change in the future if it's desired vs updating ~25 spots.

Copy link
Contributor Author

@oddballdave oddballdave Dec 10, 2024

Choose a reason for hiding this comment

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

Agree, I will make this change once we get resolution on the navbar spacing.

onActionPressed: () => updateBankInfo(updateBankData, mutateOptions),
})
}
Expand Down
Loading