Refactor ModuleRecoveryAlert to new notification system. #10398
Conversation
|
Build files for 50c44a6 have been deleted. |
There was a problem hiding this comment.
Thanks @benbowler, this is off to a good start and seems to be working well too! I've left a few points to address throughout but let's try to simplify the component a bit more in the process here. LMK if you have any questions 👍
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/index.js
Outdated
Show resolved
Hide resolved
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/index.js
Outdated
Show resolved
Hide resolved
| 'module-recovery-alert': { | ||
| Component: ModuleRecoveryAlert, | ||
| priority: 320, |
There was a problem hiding this comment.
All "alerts" such as this should be a higher priority (closer to 0) than informational notifications. Currently the zero data notification shows before this, which is a lower priority. The order should be preserved as it is today as in BannerNotifications (2nd only to AdSenseAlerts).
There was a problem hiding this comment.
I've updated it relative to the other notification of this type, I could take it closer to 0 if needed. I also left a note on the AdSenseAlerts to ensure this becomes higher priority and other refactors in this notification area are lower priority.
There was a problem hiding this comment.
Thanks, I think something like 145 would be more appropriate (less important than setup errors, but more important than scope alerts).
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/index.stories.js
Show resolved
Hide resolved
|
|
||
| export default function ModuleRecoveryAlert() { | ||
| export default function ModuleRecoveryAlert( { id, Notification } ) { |
There was a problem hiding this comment.
Most components are simplified during the transition to the new API, however this one has grown substantially, at least in terms of total lines. Perhaps this is mostly due to my previous comment about using Description components instead of defining inline here, but even beyond that, it seems like there is room for some additional refactoring to simplify things further.
There was a problem hiding this comment.
I've much reduced the size, although it is larger as the props which were once on the BannerNotification component are now duplicated on CTA/Dismiss components.
I've also added additional functionality to disable the Recover button when no modules are selected.
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/index.js
Outdated
Show resolved
Hide resolved
| isSaving={ recoveringModules } | ||
| /> | ||
| <CTALinkSubtle | ||
| id={ id } |
There was a problem hiding this comment.
Any reason why we aren't using the standard <Dismiss> component in all of these cases? Then we don't need to pass onCTAClick to dismissNotification.
Actually, we probably can use <ActionsCTALinkDismiss> as they provide a combination of CTALink and Dismiss.
There was a problem hiding this comment.
I've refactored to use this component. One thing to note is that, because we can't vary the dismissExpires independently between the CTA and Dismiss CTA the the notification won't show again if the error recurs within one day.
There was a problem hiding this comment.
I think using the <ActionsCTALink> component might fix these gap changes between the primary and secondary CTAs in all of these VRTs.
…efactor-module-recovery-alert.
…ismiss logic and disable Recover button when modules are uncheked.
| @@ -108,7 +107,6 @@ export default function BannerNotifications() { | |||
| return ( | |||
| <Fragment> | |||
| { adSenseModuleActive && <AdSenseAlerts /> } | |||
| <ModuleRecoveryAlert /> | |||
There was a problem hiding this comment.
@jimmymadon shouldn't we be migrating these from the bottom up?
There was a problem hiding this comment.
Thanks @benbowler – I think this is close to ready although I noticed that this is diverging a bit from our approach to the refactoring in terms of the order of components that we're touching. I'll let @jimmymadon confirm, but this may need to be parked for a bit while we address the others between this one and the <Notifications> below it.
| dismissLabel={ __( 'Remind me later', 'google-site-kit' ) } | ||
| dismissOnCTAClick={ shouldDismissOnCTAClick } | ||
| dismissExpires={ DAY_IN_SECONDS } | ||
| ctaDismissOptions={ { skipHidingFromQueue: false } } |
There was a problem hiding this comment.
skipHidingFromQueue: false shouldn't be needed anywhere, is it?
There was a problem hiding this comment.
We need this currently because of CTALink which sets the default value to true:
.. and ActionsCTALinkDismiss uses CTALink. We can remove this default value and update any uses of this component that require skip hiding to pass true explicitly. I think this should be a follow up issue though as it requires and audit of uses of the component and testing to confirm each of these notifications behaviour is retained.
|
@aaemnnosttv I've made a significant refactor and simplification of this component to remove nested if/else and then took it further to split out the actions and description into their own components, let me know what you think. One effect of removing the loading bar is if you have user recoverable modules the copy and checkboxes change once the module access resolvers resolve: Screen.Recording.2025-03-12.at.10.59.20.movDo you think this is appropriate or should we await resolution of the module access checks before rendering the entire component or parts of it? Or add back a loading bar, what do you think? |
…from SimpleNotification.
|
Size Change: +733 B (+0.04%) Total Size: 2.09 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks @benbowler – this is is on the right track. I don't want to burn time here but I've left a few more points to address. LMK if you have any questions 👍
| const hasMultipleRecoverableModules = useMemo( | ||
| () => Object.keys( recoverableModules || {} ).length > 1, | ||
| [ recoverableModules ] | ||
| ); | ||
| const hasUserRecoverableModules = useMemo( | ||
| () => !! Object.keys( userAccessibleModules || {} ).length, | ||
| [ userAccessibleModules ] |
There was a problem hiding this comment.
- +1 to "user recoverable modules" as this is what I would have used as well. Please also update
userAccessibleModulesabove as well since name is more accurate and applies there as well. - I'm not sure
useMemois providing a benefit here when used with[ userAccessibleModules ]as the dependencies since it returns a new array every time (so it's likely never returning the memoized value). I think it should be okay to skip using this hook here anyways since the computation is probably less expensive than the overhead of the hook.
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/index.js
Outdated
Show resolved
Hide resolved
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/index.js
Outdated
Show resolved
Hide resolved
| recoverableModules[ | ||
| Object.keys( recoverableModules || {} )[ 0 ] | ||
| ]?.name |
There was a problem hiding this comment.
If there's only one, we can grab the first without using the key.
| recoverableModules[ | |
| Object.keys( recoverableModules || {} )[ 0 ] | |
| ]?.name | |
| Object.values( recoverableModules )[ 0 ].name |
There was a problem hiding this comment.
Ternary is required to prevent crash when component unmounts after recovery dismisses the notification.
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/Actions.js
Outdated
Show resolved
Hide resolved
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/Actions.js
Outdated
Show resolved
Hide resolved
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/Actions.js
Outdated
Show resolved
Hide resolved
assets/js/components/dashboard-sharing/ModuleRecoveryAlert/Actions.js
Outdated
Show resolved
Hide resolved
| 'dashboard-sharing' | ||
| ); | ||
| } ); | ||
|
|
||
| const userAccessibleModules = useSelect( ( select ) => { |
There was a problem hiding this comment.
I realize this is from before, but I noticed it in some of the new components. There is a mismatch of types which can easily happen looking at the names:
recoverableModules({ [key: string]: Object }) – subset ofgetModulesuserAccessibleModules(string[]) – list of module slugs: subset ofrecoverableModulesthat the user has access to
Both are *Modules but different types which is confusing. For a list of slugs a name like *ModuleSlugs would be much more clear 👍 Even once we start integrating TypeScript, we should still use the same level of care when naming things.
TL;DR If we have a use for user recoverable module objects, then we can remove the slug mapping at the end and move that outside to a new variable.
|
Thanks @aaemnnosttv it makes sense to get this right while we are here as it saves management overhead of addressing at a later date and is a key goal of the epics. I've updated the implementation based on your notes. |
There was a problem hiding this comment.
Thanks @benbowler – I think we're quite close here although there are a few things left to clean up. I'll push a few changes that I see and if those look good to you I think this should be good to go.
Speaking with @jimmymadon, I think we may need to time the merging of this though given the order we should follow to avoid introducing a regression in the process, but I think we may only need to wait for one issue that he's doing now.
|
|
||
| // Only allow the alert to be dismissed if all recoverable modules are selected. | ||
| const shouldDismissOnCTAClick = | ||
| Object.keys( userRecoverableModulesSlugs || [] ).length === |
There was a problem hiding this comment.
This is probably left behind from before, but not working as expected. Object.keys on an array will work but returns an array of numeric indexes.
| const updateCheckboxes = useCallback( | ||
| ( slug ) => |
There was a problem hiding this comment.
The use of useCallback here is nullified by it's use below where it's called from an inline arrow function.
| const hasMultipleRecoverableModules = useMemo( | ||
| () => Object.keys( recoverableModules || {} ).length > 1, | ||
| [ recoverableModules ] | ||
| ); | ||
| const hasUserRecoverableModules = useMemo( | ||
| () => !! Object.keys( userRecoverableModulesSlugs || {} ).length, | ||
| [ userRecoverableModulesSlugs ] |
There was a problem hiding this comment.
Looks like these useMemos were missed, also userRecoverableModulesSlugs is treated as an object when it is an array.
| userRecoverableModuleSlugs, | ||
| hasMultipleRecoverableModules, | ||
| } ) { | ||
| const [ selectedModuleSlugs, setSelectedModuleSlugs ] = useState( null ); |
There was a problem hiding this comment.
I changed the checked object to a list of selected slugs. I think it makes the onChange much more clear as well as the function which handles the recover CTA. Everywhere else we were constantly converting it to a list of keys and filtering to those that were "checked" so it made a lot more sense to store it as this list instead.
| const [ selectedModuleSlugs, setSelectedModuleSlugs ] = useState( null ); | ||
| const [ inProgress, setInProgress ] = useState( false ); | ||
|
|
||
| // TODO: Extract to selector. |
There was a problem hiding this comment.
Noting some selectors we can extract later.
|
|
||
| if ( modules === undefined ) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const accessibleModules = Object.keys( modules ).map( ( slug ) => ( { | ||
| const slugAccessEntries = Object.keys( modules ).map( ( slug ) => [ |
There was a problem hiding this comment.
More accurately named: this is a collection of slugs with their "has access" value, but the previous name implied they were all accessible.
|
|
||
| const userAccessibleModules = useSelect( ( select ) => { | ||
| const modules = select( CORE_MODULES ).getRecoverableModules(); | ||
| // TODO: Extract to selector. |
@benbowler about this – ideally we'd have a better loading state (i.e. a skeleton loader) but that's out of scope here, and we've already done a lot so let's tackle that in a follow up. For now, let's restore the loading bar as a temporary solution, but it's not a high priority to fix because this is not a common scenario. |
|
Thanks @aaemnnosttv I've tested your changes, added back the ProgressBar and tested to check everything is working as expected. I created the follow up #10448. |
There was a problem hiding this comment.
Thanks @benbowler – this is good to go now, thanks for your patience. I just want to make sure we're clear to merge this first considering the order in which these need to land.
|
E2E failures are unrelated 👍 |
Summary
Addresses issue:
ModuleRecoveryAlertto use the new Notifications datastore #9299Relevant technical choices
Notes:
SimpleNotificationwas scaling strangely and fixed here I believe this is correct for all future and current uses of this notification type because it does not have a support for a right hand column for a graphic.checkboxesis being setup as this was simpler than adding inline terneries for all uses of the checkboxes list.checkRequirementsas this already awaits resolution of thegetRecoverableModulesselector.sitekit/no-storybook-scenario-labelwarnings as these will be refactored in Tidy up and remove redundant use oflabelproperty in VRT scenarios #9103.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist