Enhancement/9889 Refactor RRM Setup CTA Banner #9938
Conversation
|
Build files for e4c4f28 are ready:
|
|
Size Change: -1.99 kB (-0.1%) Total Size: 1.98 MB
ℹ️ View Unchanged
|
…sing itto the action.
…fication registration.
There was a problem hiding this comment.
Looking pretty good @jimmymadon – I left a few comments according to what I see so far 👍
| @@ -67,7 +72,7 @@ export default function Dismiss( { | |||
| onClick={ handleDismiss } | |||
| disabled={ disabled } | |||
| > | |||
| { dismissLabel } | |||
| { isDismissalFinal ? dismissLabel : dismissLabelInitial } | |||
There was a problem hiding this comment.
Can we maybe do this in the individual notifications instead? That way we only need to pass through a dismissLabel and this common component remains simple. This isn't a common case, so I don't feel like it really belongs in the common component.
| import { CORE_NOTIFICATIONS } from '../../../../googlesitekit/notifications/datastore/constants'; | ||
| import NotificationWithSVG from '../../../../googlesitekit/notifications/components/layout/NotificationWithSVG'; | ||
| import Description from '../../../../googlesitekit/notifications/components/common/Description'; | ||
| import LearnMoreLink from '../../../../googlesitekit/notifications/components/common/LearnMoreLink'; | ||
| import ActionsCTALinkDismiss from '../../../../googlesitekit/notifications/components/common/ActionsCTALinkDismiss'; |
There was a problem hiding this comment.
These should be imported from googlesitekit-notifications, no? We probably need to expose them but otherwise this will get duplicated in the various bundles.
There was a problem hiding this comment.
This is interesting. Perhaps I'll create a separate issue as this applies to all refactored notifications and will end up being a larger code change!
| const rrmConnected = await resolveSelect( | ||
| CORE_MODULES | ||
| ).isModuleConnected( READER_REVENUE_MANAGER_MODULE_SLUG ); | ||
| if ( isFeatureEnabled( 'rrmModule' ) ) { |
There was a problem hiding this comment.
I think we can just return early here rather than wrap everything since all of these notifications would not be registered, correct? If we did have any that should still be registered, we could add them above the return instead.
There was a problem hiding this comment.
I would keep this as is only to keep things consistent as this is the pattern we are (and have been) following for all notifications and widgets registration. The additional indentation and wrapper is clearer than the "order" (before/after the feature flag return) especially if the list gets biggers.
| const getBannerSVG = () => { | ||
| if ( breakpoint === BREAKPOINT_SMALL ) { | ||
| return SetupMobileSVG; | ||
| } | ||
|
|
||
| if ( breakpoint === BREAKPOINT_TABLET ) { | ||
| return SetupTabletSVG; | ||
| } |
There was a problem hiding this comment.
This doesn't really need to be a function, but a map which could be defined outside of the component.
e.g.
const breakpointSVGMap = {
[ BREAKPOINT_SMALL ]: SetupMobileSVG,
// ...
}
// In component...
SVG={ breakpointSVGMap[ breakpoint ] || SetupSVG };| isNotificationRetryFinal: createRegistrySelector( | ||
| ( select ) => ( state, id ) => { | ||
| const notification = | ||
| select( CORE_NOTIFICATIONS ).getNotification( id ); | ||
|
|
||
| // If a notification does not have retries, it always will be on its final render. | ||
| if ( notification.dismissRetries === 0 ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
What do you think about considering dismissible here as well? What if the notification isn't dismissible? Maybe it should trigger an invariant violation? I feel like true or false would both be incorrect 🤔
Also, maybe Retry should be Dismiss?
There was a problem hiding this comment.
Originally, the idea here was for the selector to be used in the common <Dismiss> component that would've been used by all notifications. Added an invariant now like you suggest.
Summary
Addresses issue:
ReaderRevenueManagerSetupCTABannerto not display simultaneously with other CTAs #9889Relevant technical choices
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