-
Notifications
You must be signed in to change notification settings - Fork 286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement <PublicationApprovedOverlayNotification>
component
#8843
Comments
Thank you for the IB, @ankitrox ! Please take a look at my comments below:
We'd want to add this constant to the same component file and export it as well. For reference, see
We should further expand on the conditions for the
Finally, we want to render this component in
The link to the Figma designs here points to a different mock. It is sufficient to mention that the Figma designs from the ACs should be referenced.
Similar to other issues, the link should use the Please let me know if you have any questions, thanks! |
Thanks for the IB review @nfmohit . I've made the suggested changes in IB. Assigning back to you for review. |
Thank you for updating the IB, @ankitrox ! I've left a few follow-up comments:
Similar to other overlay notification components, I think this should reside in the component file itself, i.e.
I think this statement is no longer necessary as we're handling the rendering of the component in this issue.
This constant doesn't currently exist, so it should be added to Please let me know what you think, thank you! |
Sorry @nfmohit , I was initially bit confused with both constants, but I've made it clear in IB now that one is for notification ID and other is for UI store. Updated the IB according to suggestions. Thank you. |
Looks fantastic now, thank you @ankitrox ! IB LGTM 👍 ✅ |
I've asked Sigal to help us with the SVG issue. All other comments have been addressed. Keeping this ticket here till we hear back from Sigal about SVG. |
QA Update:
|
Thank you for sharing your observations, @wpdarren !
Hmm, while this wasn't a part of the ACs, now that I take a look at the behaviour, it appears that dismissing the notification after the publisher center is opened in a new tab will be the appropriate behaviour. I'll open a quick follow-up PR to address this.
The URL is correct. The rest of the parts are actually added by Publisher Center themselves. |
QA Update: ✅Verified:
|
Feature Description
A
<PublicationApprovedOverlayNotification>
component should be implemented for the Reader Revenue Manager module that renders an<OverlayNotification>
component in the Site Kit dashboard.Screenshot for reference
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
<PublicationApprovedOverlayNotification>
component should be added for the Reader Revenue Manager module according to the Figma designs that renders an overlay notification that lets the user know that their publication was approved.rrmModule
feature flag is enabled AND a key in thecore/ui
store is set totrue
, e.g.show-rrm-publication-approved-notification
(with constantUI_KEY_SHOW_RRM_PUBLICATION_APPROVED_NOTIFICATION
).Implementation Brief
UI_KEY_SHOW_RRM_PUBLICATION_APPROVED_NOTIFICATION
toassets/js/modules/reader-revenue-manager/datastore/constants.js
with value'show-rrm-publication-approved-notification'
.assets/js/module/reader-revenue-manager/components/PublicationApprovedOverlayNotification.js
RRM_PUBLICATION_APPROVED_OVERLAY_NOTIFICATION
with valuerrmPublicationApprovedOverlayNotification
. This will be passed asnotificationID
toOverlayNotification
component.shouldShowNotification
according to following points. This prop will be passed toOverlayNotification
. Note that this should fulfill all of the following conditions to betrue
.RRM_PUBLICATION_APPROVED_OVERLAY_NOTIFICATION
to check this.useViewOnly
hook).useDashboardType
hook).UI_KEY_SHOW_RRM_PUBLICATION_APPROVED_NOTIFICATION
key fromCORE_UI
store is set totrue
. This will be set totrue
bysyncPublicationOnboardingState
action (refer Implement RRMsyncPublicationOnboardingState()
action #8796).GraphicDesktop
andGraphicMobile
according to the figma designs.useBreakpoint
hook do position the CTAs.dismissNotification
in componentMaybe later
should calleddismissOverlayNotification
fromCORE_UI
store. PassRRM_PUBLICATION_APPROVED_OVERLAY_NOTIFICATION
todismissOverlayNotification
.Enable features
CTA should useButton
component withhref
prop passed to it. The link should be be passed usinggetServiceURL
selector (Implement RRMgetServiceURL()
selector #8848), pass publication ID to the selector.isDismissingItem
selector fromCORE_USER
store.assets/js/components/OverlayNotification/OverlayNotificationsRenderer.js
, renderrrmPublicationApprovedOverlayNotification
if RRM feature is enabled (referuseFeature
hook).Test Coverage
PublicationApprovedOverlayNotification
, also test dismissal behaviour.PublicationApprovedOverlayNotification
component.QA Brief
Make sure module is enabled in tester plugin.
Activate the module and go to
Site Kit > Dashboard
. Do not do any configuration as of now.Run the following command in browser console.
As soon as above command is run, it should display the overlay notification.
"Enable Features" should take to the publisher centre.
Changelog entry
The text was updated successfully, but these errors were encountered: