Update ModuleRecoveryAlert to show with notices

[eclarke1]

Bug Description

Asana bug bash issue: https://app.asana.com/0/1202258919887896/1202583202387112 please see Asana task for background and additional comments.

It's possible to encounter multiple stacked notices, as per the screenshot within this task.

In my case I never dismissed the "Congrats" notice, and then from another admin account disconnected from SK. When revisiting and refreshing the dashboard for admin2, I encounter the module recovery notice below the congrats notice.

See #4689 for context

Steps to reproduce

1. Trigger a module setup success notice (save the URL for later)
2. Share a module with admin 1
3. Disconnect admin 1
4. Sign in with admin 2
5. Visit saved URL from before with success notice
6. See congrats message + module recovery alert

Screenshots

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• The module recovery alert should be relocated within <BannerNotifications> while preserving the same conditions for display (one exception here is the isDashboard check as banner notifications are only shown on the main dashboard)
  • Order is significant, and so the module recovery alert should be moved right after CoreSiteBannerNotifications
• The module recovery alert should be updated so that it can be dismissed
  • The text for the dismissal should read Remind me later
  • The dismissal should last for one day

Implementation Brief

• Start with this branch which implements most of the ACs above by moving ModuleRecoveryAlert into BannerNotifications.
• The 'Dismiss' CTA shows up as a button instead of a link. This is because within the <ModuleRecovery> component, the "Recover" Button component is defined and passed down within the children prop of BannerNotification instead using its ctaLabel/onCTAClick props. To fix this, define ctaLabel and onCTAClick only when the "Recover" <Button> component is defined within the children variable. If defined, the ctaLabel should always be __( 'Recover', 'google-site-kit' ) and the onCTAClick prop should always be handleRecoverModules. Otherwise they should be false. Pass these two variables as props to the BannerNotification component.
• Modify handleRecoverModules so that the callback returns an object { dismiss: false }. Within <BannerNotification>, modify handleCTAClick so as to expect an object from onCTAClick(), i.e. const { dismiss = true } = await onCTAClick( e ) || {}. Read this comment for better context.
• Within <BannerNotification>, add the <Spinner> component after the ctaLink Button component (and before the DismissLinkComponent). Use a local state variable, say showSpinner which by default is false. Set this variable to true within the handleCTAClick() function, before the call to onCTAClick. Set this back to false after the call as mentioned in this comment. Use the same state variable to disable the CTA button.
• Ensure existing storybook stories render the CTA and dismiss links correctly.

Test Coverage

• No new tests required.

QA Brief

• Follow steps to reproduce and verify the notices now appear "behind" the setup success notifications.
• Additionally, verify that the "Remind Me Later" appears as 'Link' next to the 'Recover' button but hides itself when the 'Recover' button is clicked and the spinner appears. Also, if there is no 'Recover' button, the 'Remind me later' becomes the main button.
• Verify any errors when recovering modules (newly added in Add Error Handling to ModuleRecoveryAlert and show error message to the user. #5318) are displayed correctly (use the last two steps in that issue's QAB).
• Verify other notifications and their action buttons have not changed and work normally as before.

Changelog entry

• Update location of module recovery alert to be grouped with normal notifications.

[techanvil]

Hi @jimmymadon, I've spotted a few issues with this IB.

A couple of minor ones - the link to the branch doesn't work, and <ModuleRecovery> should read <ModuleRecoveryAlert>.

More substantially though, I can see some problems with the proposed approach of rendering the Button via BannerNotification's CTA props.

Note that although we want to enable the dismiss button, we don't want the Recover button to dismiss the banner - this would cause problems in usage, as we need the banner to stick around after pressing Recover in order to show any errors or remaining unrecovered modules, etc.

However in order to show the dismiss button the isDismissable prop needs to be true - but when it's true the handleCTAClick callback will dismiss the banner on pressing the CTA.

Additionally, in order to display the CTA the ctaLink prop also needs to be set. However this is not ideal as we don't want to render the href at all here. We could take a slighly hacky approach of passing a link in (e.g. #) and then calling event.preventDefault() in the onCTAClick callback - but it would be preferable to avoid this. An alternative could be to switch the logic to check ctaLabel instead of ctaLink but this would be risky and some solid due diligence would be needed to ensure it doesn't introduce any problems to any existing use of BannerNotification.

What to do?

Taking the above into consideration, it occurs to me that a viable solution could be to leave the Recover button as it is now, and instead pass a new prop into BannerNotification called dismissStyle (or something like that) which could have the value button or link. Then, if this prop is present it can override the logic for determining which DismissComponent to use for the dismiss button, in this case specifying link to achieve the desired effect.

---

However...

There is one more thing we might want to consider which is the spinner placement. If we go ahead with the dismissStyle approach without further changes, we end up with the spinner being displayed between the two buttons. Beyond the simple question of placement, this is not perfect from a UX perspective as either the Remind me later text will shift to the right when the spinner is displayed, or we can keep the distance between the buttons fixed but it'll be inconsistent with other banners.

An alternative placement could be to the right of both buttons, it looks slightly out of place at to me at first glance but is probably OK, however this would require more changes to BannerNotification, adding something like a showSpinner prop or maybe Footer, ButtonWrapper, that sort of thing.

Or maybe we could consider a totally different placement. I'm not sure about this, so am going to ask @aaemnnosttv for his thoughts on this one.

[aaemnnosttv]

Thanks @techanvil – there are indeed some tricky bits to this that I didn't foresee.

It seems to me that the first part which is making this more difficult than it needs to be is the added spinner, but we'll come back to that. I feel like the Recover action should be able to work within the notice's onCTAClick prop, the only problem there (that you also identified) is that this will dismiss the notification where we essentially only want it to be dismissible via the explicit choice rather than via the CTA. It would arguably be a better experience if the banner didn't disappear after recovering all the modules anyways as there is otherwise no indication what happened, whether it was successful or not, etc but this is getting a bit out of scope here.

In this case, I think we can work around the dismissal by allowing the onCTAClick to opt-out by returning a kind of "result" object like { dismiss: false }, although by default it would still dismiss as today. E.g.

const { dismiss = true } = await onCTAClick( e ) || {}

The only part left to address is the spinner, which can probably move to the BannerNotification next to the cta's Button as this will be handy for any other async CTA action as well. This could then be displayed right before invoking the onCTAClick and disabled afterwards, that way it would be displayed as long as the onCTAClick function was in progress. Regarding the shift that happens when it appears, this can be addressed by hiding the dismiss link while it is being shown, as there is no reason for them to be present at the same time (a user shouldn't be able to dismiss the notification while the CTA is in progress).

I think that addresses everything here, and going forward ModuleRecoveryAlert will be a bit less of a special case by leveraging the existing cta props rather than exposing it via children.

[techanvil]

Thanks @techanvil – there are indeed some tricky bits to this that I didn't foresee.

It seems to me that the first part which is making this more difficult than it needs to be is the added spinner, but we'll come back to that. I feel like the Recover action should be able to work within the notice's onCTAClick prop, the only problem there (that you also identified) is that this will dismiss the notification where we essentially only want it to be dismissible via the explicit choice rather than via the CTA. It would arguably be a better experience if the banner didn't disappear after recovering all the modules anyways as there is otherwise no indication what happened, whether it was successful or not, etc but this is getting a bit out of scope here.

In this case, I think we can work around the dismissal by allowing the onCTAClick to opt-out by returning a kind of "result" object like { dismiss: false }, although by default it would still dismiss as today. E.g.

const { dismiss = true } = await onCTAClick( e ) || {}

The only part left to address is the spinner, which can probably move to the BannerNotification next to the cta's Button as this will be handy for any other async CTA action as well. This could then be displayed right before invoking the onCTAClick and disabled afterwards, that way it would be displayed as long as the onCTAClick function was in progress. Regarding the shift that happens when it appears, this can be addressed by hiding the dismiss link while it is being shown, as there is no reason for them to be present at the same time (a user shouldn't be able to dismiss the notification while the CTA is in progress).

I think that addresses everything here, and going forward ModuleRecoveryAlert will be a bit less of a special case by leveraging the existing cta props rather than exposing it via children.

Thanks for taking a look at this @aaemnnosttv. I like your proposed solution, bringing the spinner into BannerNotification like that seems like a useful addition. The only thing I am not sure about it how the positioning will look in practice with the dismiss button disappearing; if, say, the request only takes a fraction of a second then the effect might look like the dismiss button flickering in and out and might be a bit odd. However I do think we should try it out, and we'd be in a good position to iterate on it if needs be.

[aaemnnosttv]

The only thing I am not sure about it how the positioning will look in practice with the dismiss button disappearing; if, say, the request only takes a fraction of a second then the effect might look like the dismiss button flickering in and out and might be a bit odd. However I do think we should try it out, and we'd be in a good position to iterate on it if needs be.

That's a good point, although in most cases I would expect a user would be recovering all available modules (as the default unless they unselected some), in which case the entire notification would disappear so I think this is an acceptable edge case for now. I would argue this flicker from fast requests is possible in many other places today as well. IMO it would be good to introduce a small amount of artificial latency in such cases as a kind of minimum visible time to improve the stability of the interface, but I'd say that's out of scope here.

[jimmymadon]

@aaemnnosttv @techanvil Thanks both for these suggestions - have updated the IB to incorporate them. 👍

For some of these reasons, we can consider using buttons which show the loading spinner within the button itself preventing any awkward shifts or needing any additional space.

[aaemnnosttv]

Thanks @jimmymadon – this looks good overall, just one question.

• Within <BannerNotification>, create a new prop ctaDisabled. It should accept a boolean value which disables the <Button> component that renders the CTA Link within the notification (googlesitekit-notification__cta). Use this new prop to pass the logic of disabling the "Recover" <Button> component within ModuleRecoveryAlert down to BannerNotification, similar to passing the ctaLabel and onCTAClick props.

Do we need an added prop for this? Are you suggesting this would set the disabled on the button or prevent it from being rendered? If we only want to disable it while it's in progress we should be able to use the same showSpinner state for that, otherwise we'd be duplicating logic in the parent. This would only be needed for async CTA actions as well.

[jimmymadon]

@aaemnnosttv Makes sense since we've moved the spinner to Banner Notification. Thanks - have updated the IB.

[aaemnnosttv]

Great, thanks @jimmymadon 👍

IB ✅

[wpdarren]

QA Update: ✅

Verified:

• Confirmed that the notices now appear behind the setup success notifications.
• The Remind Me Later appears as 'Link' next to the 'Recover' button but hides itself when the 'Recover' button is clicked and the spinner appears.
• Confirmed if there is no Recover button, the Remind me later becomes the main button. I wasn't able to recreate this with the recovery alert, but did with the zero data notification where the Remind Me Later link is now a main button.
• Verified any errors when recovering modules are displayed correctly
• Verified other notifications and their action buttons have not changed and work normally as before.

Screencasts and screenshots

alert-1.mp4

alert-2.mp4