feat: migrate from Notifications to Banners (M2-6946) #477
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Based on same infrastructure used by Admin App, but refactored to follow web app conventions. Included `useBanners` hook for handy convenience functions.
Same as Admin App, uses MUI's Alert component. Override default theme styling of Alert to align with Figma. Create `useWindowFocus` hook, copied verbatim from Admin App, to uphold a11y UX.
Create `Banners` wrapper component to replace `NotificationCenter`. Same as in Admin App, use default duration of 3.5s for success banners.
Replace instances of `NotificationCenter` with `Banners`, and `useNotification` with `useBanners`.
Copied almost verbatim from Admin App. Required incorporating `@testing-library/user-event` package into project, and upgrading `@vitejs/plugin-react` to latest version, as the current version was buggy and causing tests involving rendering React components to fail. Add `renderWithProviders` test utility (based on same existing utility in Admin App) to be able to prepopulate Redux state in tests, required by `Banners` component. Also add `expectBanner` utility for testing presence/absense of banners, copied from Admin App.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Including Redux `banners` state selector in `useBanners` hook causes infinite state update loop in React; simplified the hook so that only the one component needing access to the `banners` array requests it.
Admin App needed the same solution; I had oversimplified the code when migrating it from the Admin App and learned this import structure is indeed required.
Same as Admin App, remove any displayed banners upon logout. Required migrating `renderHookWithProviders` testing utility.
farmerpaul
commented
Jun 10, 2024
ChaconC
approved these changes
Jun 10, 2024
There was a problem hiding this comment.
LGTM and all tests worked, great work!
I did run into a case where the banner stayed on screen indefinitely, but that was caused by a popup from my password manager (never triggering window focus to change).
A minor comment about removing banners on success but feels ok to pre-approve.
Ensures login & signup forms do not display persistent error banners after forms have been successfully submitted. Also add convenience functions to `useBanners` to make removing such banners easier.
sultanofcardio
approved these changes
Jun 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📝 Description
🔗 Jira Ticket M2-6946
This PR replaces the
NotificationCenter,useNotificationhook, and related infrastructure withBanners,useBanners, adapted from the Admin App with only slight changes to align with the Web App's code structure.Includes:
Bannerbase component (replacesNotificationcomponent)createThemeuseWindowFocushook (copied verbatim from Admin App) required for enhanced a11y/UXBannerscontainer component (replacesNotificationCentercomponent)SuccessBanner,WarningBanner,ErrorBanner, andInfoBanneruseBannershook that provides convenience functions for adding/removing banners (replacesuseNotificationhook)useSessionBannershook, adapted from Admin App, which removes any displayed banners upon logoutBannerandBannerscomponents@testing-library/user-eventpackage and upgrading@vitejs/plugin-reactto the latest version to fix a bug with executing tests that involved rendering React componentsTip
It may be easier to review this PR commit-by-commit due to the number of changes.
📸 Screenshots
🪤 Peer Testing
Requires
yarn installExpected outcome: An error banner should render with updated styling, including a close button.
Expected outcome: Banner should disappear.
Expected outcome: Banner should disappear.
Expected outcome: Banner should remain visible.
Expected outcome: Banner should still be visible.
Expected outcome: Banner should disappear.
Expected outcome: Error banner should should render with updated styling.
Expected outcome: Error banner should disappear.