feature/8467-UseDesignSystemAlertComponent #9022
Conversation
alexandec
requested review from
timwright12,
theodur,
dumathane,
cadibemma and
Sparowhawk
| header={t('downtime.title')} | ||
| headerA11yLabel={t('downtime.title')} |
There was a problem hiding this comment.
Even though this is migrated from what was already used, I don't think we need the headerA11yLabel prop here
VAMobile/src/components/WhatsNew.tsx
Outdated
| header={t('whatsNew.title')} | ||
| headerA11yLabel={t('whatsNew.title')} |
There was a problem hiding this comment.
Same here, the headerA11yLabel prop isn't needed
| header={t('fileUpload.accessibilityAlert.title')} | ||
| headerA11yLabel={t('fileUpload.accessibilityAlert.title')}> |
There was a problem hiding this comment.
Same here, the headerA11yLabel prop isn't needed
| header={t('prescription.details.banner.title')} | ||
| headerA11yLabel={t('prescription.details.banner.title')} |
There was a problem hiding this comment.
Same here, the headerA11yLabel prop isn't needed
| header={t('prescription.history.transferred.title')} | ||
| headerA11yLabel={t('prescription.history.transferred.title')} |
There was a problem hiding this comment.
Same here, the headerA11yLabel prop isn't needed
| header={headerText} | ||
| headerA11yLabel={headerText} |
There was a problem hiding this comment.
Same here, the headerA11yLabel prop isn't needed
| header={t('secureMessaging.viewMessage.errorTitle')} | ||
| headerA11yLabel={t('secureMessaging.viewMessage.errorTitle')} |
There was a problem hiding this comment.
Same here, the headerA11yLabel prop isn't needed
| header={t('editAddress.validation.verifyAddress.title')} | ||
| headerA11yLabel={t('editAddress.validation.verifyAddress.title')} |
There was a problem hiding this comment.
Same here, the headerA11yLabel prop isn't needed
| expandable={true} | ||
| focusOnError={false} |
There was a problem hiding this comment.
At first, I was going to suggest adding logic to the AlertWithHaptics component that sets focusOnError to false by default when exxpandable is true since the CollapsibleAlert didn't ever focus on error, but I actually like having the flexibility to be able to set focusOnError even for expandable alerts. We don't do this anywhere yet, but it's nice to be able to do that if we need to
github-actions
bot
added
FE-With QA
VAMobile/package.json
Outdated
| @@ -91,7 +91,7 @@ | |||
| "@babel/preset-env": "^7.24.7", | |||
| "@babel/preset-typescript": "^7.24.7", | |||
| "@babel/runtime": "^7.24.7", | |||
| "@department-of-veterans-affairs/eslint-config-mobile": "0.15.0", | |||
| "@department-of-veterans-affairs/eslint-config-mobile": "0.18.0", | |||
There was a problem hiding this comment.
Did you want to bump this to be issuing linting warnings for Loading indicator? Not sure if that had been prioritized yet.
There was a problem hiding this comment.
Good call, I moved this back to 0.15.0
| @@ -405,11 +405,11 @@ function ViewMessageScreen({ route, navigation }: ViewMessageScreenProps) { | |||
| )} | |||
| {replyExpired && ( | |||
| <Box my={theme.dimensions.standardMarginBetween}> | |||
| <AlertBox border={'warning'} title={t('secureMessaging.reply.youCanNoLonger')}> | |||
| <AlertWithHaptics variant="warning" header={t('secureMessaging.reply.youCanNoLonger')}> | |||
| <TextView mt={theme.dimensions.standardMarginBetween} variant="MobileBody"> | |||
| {t('secureMessaging.reply.olderThan45Days')} | |||
There was a problem hiding this comment.
Could potentially be simplified moving this to the description prop to AlertWithHaptics.
| @@ -511,26 +511,26 @@ function EditDraft({ navigation, route }: EditDraftProps) { | |||
| function renderAlert() { | |||
| return ( | |||
| <Box my={theme.dimensions.standardMarginBetween}> | |||
| <AlertBox border={'warning'} title={t('secureMessaging.reply.youCanNoLonger')}> | |||
| <AlertWithHaptics variant="warning" header={t('secureMessaging.reply.youCanNoLonger')}> | |||
| <TextView mt={theme.dimensions.standardMarginBetween} variant="MobileBody"> | |||
| {t('secureMessaging.reply.olderThan45Days')} | |||
There was a problem hiding this comment.
Could potentially be simplified moving this to the description prop to AlertWithHaptics.
| @@ -51,7 +50,7 @@ function TakePhotos({ navigation, route }: TakePhotosProps) { | |||
| } | |||
|
|
|||
| const collapsibleContent = ( | |||
There was a problem hiding this comment.
Could be further simplified adding the TextView stuff to description on the Alert and passing the LinkWithAnalytics directly as children.
| <AlertWithHaptics | ||
| variant="error" | ||
| header={fixSpacingAndSpecialCharacters(titleText)} | ||
| headerA11yLabel={titleA11y}> | ||
| <Box> |
There was a problem hiding this comment.
Could potentially be further simplified to using Alert component description/primaryButton props leaving only the ClickToCallPhoneNumber as children.
github-actions
bot
added
FE-Ready to Merge
and removed
FE-With QA
alexandec
dismissed stale reviews from rbontrager, Sparowhawk, and TimRoe
via
8a7f9ec
|
@rbontrager @Sparowhawk @TimRoe had to resolve a conflict, please reapprove when you get a chance |
Description of Change
Replace the AlertBox and CollapsibleAlert components with the new design system Alert component.
Note that the Alert component doesn't contain the existing AlertBox haptics and scrolling features. I created an AlertWithHaptics wrapper component which triggers haptics and scrolls to the alert where appropriate.
Locations changed:
Screenshots/Video
Example before/after comparisons:
Testing
Reviewer Validations
PR Checklist
Reviewer: Confirm the items below as you review
For QA
Run a build for this branch