Skip to content
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 RRM module setup success banner #8840

Closed
30 tasks
nfmohit opened this issue Jun 8, 2024 · 18 comments
Closed
30 tasks

Implement RRM module setup success banner #8840

nfmohit opened this issue Jun 8, 2024 · 18 comments
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Sp Wk 2 Issues to be completed in the second week of the assigned sprint Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Jun 8, 2024

Feature Description

A subtle notification component should be used to implement the setup success banner for the Reader Revenue Manager module. The copy of the banner will vary depending on the onboarding state of the connected publication.

Screenshots for reference

Publication is awaiting review:
image

User needs to perform additional steps to complete publication setup:
image

Publication setup is complete:
image


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

Acceptance criteria

  • The module setup success banner should be implemented for the Reader Revenue Manager with the aid of a subtle notification according to the Figma designs.
  • The contents of the success notification will vary based on the onboarding status of the connected publication.
    • If the publication has completed setup and is ready for use:
      • The notification should be in the success variant.
      • Text: "Your Reader Revenue Manager account was successfully set up!"
      • Sub-text: "Unlock your full reader opportunity by enabling features like subscriptions, contributions and newsletter sign ups in the Reader Revenue Manager settings."
      • CTA: "Customize settings" (with an external link icon). This should open the publication in Reader Revenue Manager on a new browser tab.
      • Dismissal: "Maybe later". This should persistently dismiss the notification.
    • Publication is awaiting review:
      • The notification should be in the success variant.
      • Text: "Your Reader Revenue Manager account was successfully set up!"
      • Sub-text: "Your publication is still awaiting review, you can check its status in Reader Revenue Manager."
      • CTA: "Check publication status" (with an external link icon). This should open the publication in Reader Revenue Manager on a new browser tab.
      • Dismissal: "Got it". This should persistently dismiss the notification.
    • Publication needs additional steps from the user:
      • The notification should be in the warning variant.
      • Text: "Your Reader Revenue Manager account was successfully set up, but your publication still requires further setup in Reader Revenue Manager."
      • CTA: "Complete publication setup" (with an external link icon). This should open the publication in Reader Revenue Manager on a new browser tab.
      • Dismissal: "Got it". This should persistently dismiss the notification.
  • It should adhere to the mobile designs in Figma here.

Implementation Brief

Reusable SubtleNotification component

In the assets/js/components/notifications directory, create a new component, SubtleNotification.js:

  • Create a function component, SubtleNotification that renders a notification with the following props:
    • title: The title of the notification.
    • description: The description of the notification.
    • icon: The icon for the notification (optional).
    • ctaLabel: The label for the CTA button.
    • ctaLink: The URL for the CTA button.
    • onCTAClick: A callback function to handle the CTA button click.
    • ctaLinkExternal: A boolean to indicate if the CTA link should open in a new tab and show an external link icon.
    • dismissLabel: The label for the dismiss button.
    • onDismiss: A callback function to dismiss the notification.
    • variant: The variant of the notification (success or warning). By default, it should be success.
    • Add more props as needed.
  • The component should render a similar structure like other SubtleNotification components. For example, see, SetupSuccessSubtleNotification and AudienceSegmentationSetupSuccessSubtleNotification.
  • Conditionally apply the googlesitekit-subtle-notification--warning class to the notification container if the variant is warning.
  • Render the success check-fill.svg icon if the variant is success.
  • Render the warning warning.svg icon if the variant is warning.
  • If the icon prop is provided, render the provided icon.
  • Add the googlesitekit-subtle-notification--warning class styles and any other necessary styles for the component in assets/sass/components/dashboard/_googlesitekit-subtle-notification.scss.

RRMSetupSuccessSubtleNotification component

In assets/js/modules/reader-revenue-manager/components, add a new dashboard directory and create a new component, RRMSetupSuccessSubtleNotification.js:

  • Create a function component RRMSetupSuccessSubtleNotification that renders the setup success notification for the Reader Revenue Manager module.
  • Create a constant READER_REVENUE_MANAGER_SETUP_SUCCESS_NOTIFICATION OR RRM_SETUP_SUCCESS_NOTIFICATION.
  • Check if the notification has been dismissed using the isItemDismissed selector.
  • If it has been dismissed, return null.
  • Retrieve the publication onboarding status using the getPublicationOnboardingState selector.
  • Retrieve the publication ID using the getPublicationID selector.
  • Retrieve the service URL for the current publication ID using the getServiceURL selector and pass the publication ID as an argument.
  • If the publication onboarding status is ONBOARDING_COMPLETE, return the SubtleNotification component with the following props:
    • title: As per the AC.
    • description: As per the AC.
    • ctaLabel: As per the AC.
    • ctaLink: The retrieved service URL.
    • dismissLabel: As per the AC.
    • onDismiss: A callback function that dispatches the dismissItem action.
    • ctaLinkExternal: true
    • variant: success (optional).
  • If the publication onboarding status is PENDING_VERIFICATION, return the SubtleNotification component with the following props:
    • title: As per the AC.
    • description: As per the AC.
    • ctaLabel: As per the AC.
    • ctaLink: The retrieved service URL.
    • dismissLabel: As per the AC.
    • onDismiss: A callback function that dispatches the dismissItem action.
    • ctaLinkExternal: true
    • variant: success (optional).
  • If the publication onboarding status is ONBOARDING_ACTION_REQUIRED, return the SubtleNotification component with the following props:
    • title: As per the AC.
    • ctaLabel: As per the AC.
    • ctaLink: The retrieved service URL.
    • dismissLabel: As per the AC.
    • onDismiss: A callback function that dispatches the dismissItem action.
    • ctaLinkExternal: true
    • variant: warning.
  • It should match the mobile designs in Figma here.
  • Note: The source of truth for the copies should be the Figma designs.

Add the notification to the Dashboard

In assets/js/components/notifications/SubtleNotifications.js:

  • Render the RRMSetupSuccessSubtleNotification component only if the rrmModule feature flag is enabled.

Test Coverage

  • Add storybook stories for the SubtleNotification component in assets/js/components/notifications/SubtleNotification.stories.js.
  • Add storybook stories for the RRMSetupSuccessSubtleNotification component in assets/js/modules/reader-revenue-manager/components/dashboard/RRMSetupSuccessSubtleNotification.stories.js.
  • Add unit tests for the SubtleNotification component in assets/js/components/notifications/SubtleNotification.test.js.

QA Brief

Prerequisite

  • Use the latest version of the Tester plugin to change the publication onboarding state and verify the different banner notifications.
  • Disconnect the RRM module and reconnect whenever you change the publication onboarding state from the tester plugin.

  • Since Scaffold RRM module setup flow #8800 has been merged, it is now possible to setup RRM without any snippets using normal flows.
  • To fully test all variations of the success notice, we will need 3 publications with different states as described in AC.
  • After enabling rrmModule feature flag, go through the RRM setup flow. Once complete, the success notification should show up based on the status of the connected publication.
  • Ensure the secondary CTA dismisses the notification.

Changelog entry

  • Add the Reader Revenue Manager module success setup banner notification.
@nfmohit nfmohit self-assigned this Jun 8, 2024
@nfmohit nfmohit added P0 High priority Type: Enhancement Improvement of an existing feature Team M Issues for Squad 2 Module: RRM Reader Revenue Manager module related issues labels Jun 8, 2024
@nfmohit nfmohit removed their assignment Jun 8, 2024
@nfmohit nfmohit mentioned this issue Jun 8, 2024
18 tasks
@ivonac4 ivonac4 added Module: RRM Reader Revenue Manager module related issues and removed Module: RRM Reader Revenue Manager module related issues labels Jun 11, 2024
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jun 14, 2024
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Jun 26, 2024
@hussain-t hussain-t assigned hussain-t and nfmohit and unassigned hussain-t Jul 4, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jul 6, 2024

Thank you for the brilliant IB, @hussain-t ! Please take a look at my comments below:

  1. What do you think of changing the external prop for SubtleNotification to ctaLinkExternal? That way, the prop is a little more meaningful.
  2. The copy to use within the banner shouldn't be re-written in the IB, instead, the AC should be used as the source of truth. What do you think about updating them to reference the ACs?

Please let me know what you think, thank you!

@nfmohit nfmohit assigned hussain-t and unassigned nfmohit Jul 6, 2024
@hussain-t
Copy link
Collaborator

Thanks, @nfmohit. I initially included the copies in the IB to accommodate reviewers who prefer to have the text repeated for clarity. However, as suggested, I have updated the IB to reference the ACs directly. The source of truth for the copies should be the Figma design.

@hussain-t hussain-t assigned nfmohit and unassigned hussain-t Jul 8, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jul 9, 2024

Thanks @hussain-t ! IB LGTM 👍 ✅

@nfmohit nfmohit removed their assignment Jul 9, 2024
@hussain-t
Copy link
Collaborator

Hi @nfmohit, just a heads-up: we have another issue (#8725) in progress to implement a reusable SubtleNotification component. cc: @ivonac4.

@ivonac4 ivonac4 added the Sp Wk 2 Issues to be completed in the second week of the assigned sprint label Jul 26, 2024
@kuasha420 kuasha420 self-assigned this Jul 29, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ❌

I've tested this and I will document down additional ITEMS here, starting to number it as from 3 and leave item 1 and 2 as what @nfmohit has highlighted in this comment.

I am assigning this to @kuasha420 for fixes.

Meanwhile, I am also tagging @nfmohit and @wpdarren in this because we did refer to @wpdarren 's observations under this document. I am not sure if we want to create additional tickets for those or which ones should sit within this issue as items 7 onwards. Would be great if you can review this so that we can track the issues better.

ITEM 3:
This is more of a clarification.
What is the expectation for the disappearance of the banners? Currently if we reload the page, the banner remains but if we go to another page (e.g. settings) and come back to the dashboard, it will be gone.

More context in the video below.

Banner.disappears.if.go.to.another.page.but.reloading.it.s.there.mov
_______________________________________________________________

ITEM 4:
It's been highlighted in @nfmohit 's comment about the icon not being there on mobile.
I wanted to highlight the warning variant banner on mobile at 375px. Based on the figma at 375px, everything will appear correctly. However, in the actual implementation, the buttons are wrapping on 2 lines.

Figma: Screenshot 2024-08-14 at 14 50 45

Implementation:
Screenshot 2024-08-14 at 14 51 11


ITEM 5:
On tablet, the banners just look weird in my opinion.
Refer to the picture attached. There is a very dense block of text and the buttons sections have a lot of space. This feels unbalanced. I'm wondering if it might be best to move the buttons below and let the text sit along the width of the banner.

Screenshot 2024-08-13 at 18 04 07

ITEM 6:
There is this part of the QAB which states : "Ensure both the primary and secondary CTAs dismiss the notification."
I understand that even clicking the CTAs would dismiss the notification.
I wanted us to review this because I felt like it was a bad experience personally.
My suggestion would be if we click on CTAs like 'Check publication status', it's not necessary to dismiss the notification?

More context in the video below:

subtle.notification.disappears.after.click.on.customize.mov

@nfmohit nfmohit removed their assignment Aug 15, 2024
@kuasha420 kuasha420 mentioned this issue Aug 19, 2024
18 tasks
@kuasha420 kuasha420 assigned nfmohit and unassigned kuasha420 and wpdarren Aug 19, 2024
@kuasha420
Copy link
Contributor

Thank you @hussain-t @nfmohit and @kelvinballoo !

I've filed a follow up PR to fix most of the points raised. In addition-

ITEM 3 - This is consistent with the Setup Success Notifications of other modules. So this can move ahead as is. However, I think we need to show a similar notification if the publication needs further attention, once this notification is no longer displayed. Is there a issue filed for that, @nfmohit?

ITEM 5 - That's a good point, however, I couldn't find a design for tablet for Subtle Notification. Feel free to create a separate issue for this, and we can ask Sigal for a tablet design.

ITEM 6 - This was decided during CR here. I think it makes sense to dismiss the notifications on both CTA, but doesn't have a strong opinion either way. Any thoughts @nfmohit @hussain-t @techanvil ?

Thanks all!

@techanvil
Copy link
Collaborator

techanvil commented Aug 19, 2024

Hmm, actually I am not sure it's such a good idea to dismiss a notification when the CTA is an external link.

There's no way of knowing for sure if the user successfully navigated to the link when clicking on the CTA, so it would be useful to keep the notification open in case they need to click on it again. That's my 2p on the issue, anyway.

@nfmohit nfmohit assigned kuasha420 and unassigned nfmohit Aug 19, 2024
@kuasha420 kuasha420 assigned nfmohit and unassigned kuasha420 Aug 20, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Aug 20, 2024

Hmm, actually I am not sure it's such a good idea to dismiss a notification when the CTA is an external link.

There's no way of knowing for sure if the user successfully navigated to the link when clicking on the CTA, so it would be useful to keep the notification open in case they need to click on it again. That's my 2p on the issue, anyway.

@techanvil I agree. In the follow-up PR, we've removed the dismissal from the primary CTA. Thank you!

@techanvil
Copy link
Collaborator

Top stuff, thanks @nfmohit.

@nfmohit
Copy link
Collaborator Author

nfmohit commented Aug 20, 2024

Hi @kelvinballoo ! This is now back with you as we've addressed your observations with a follow-up PR:

Meanwhile, I am also tagging @nfmohit and @wpdarren in this because we did refer to @wpdarren 's observations under this document. I am not sure if we want to create additional tickets for those or which ones should sit within this issue as items 7 onwards. Would be great if you can review this so that we can track the issues better.

I think most of these concerns have been addressed, or Asana tasks have been created for them. Please let me know otherwise.

ITEM 3:
@kelvinballoo - This is more of a clarification.
What is the expectation for the disappearance of the banners? Currently if we reload the page, the banner remains but if we go to another page (e.g. settings) and come back to the dashboard, it will be gone.

@kuasha420 - This is consistent with the Setup Success Notifications of other modules. So this can move ahead as is. However, I think we need to show a similar notification if the publication needs further attention, once this notification is no longer displayed. Is there a issue filed for that, @nfmohit?

This is how the UX was defined. We will have notices in the module settings screens if the publication requires further setup/is pending verification. If you think it would be better to persist this notification until dismissed, could you please add this as a "Nice to Have" in the bug bash board? Thank you!

ITEM 4:
It's been highlighted in @nfmohit 's comment about the icon not being there on mobile.
I wanted to highlight the warning variant banner on mobile at 375px. Based on the figma at 375px, everything will appear correctly. However, in the actual implementation, the buttons are wrapping on 2 lines.

This has been addressed in a way such that if there is not enough space for the buttons (in extremely shrunk viewports), we'll stack the buttons so that one appears on top of another.

ITEM 5:
@kelvinballoo - On tablet, the banners just look weird in my opinion.
Refer to the picture attached. There is a very dense block of text and the buttons sections have a lot of space. This feels unbalanced. I'm wondering if it might be best to move the buttons below and let the text sit along the width of the banner.

@kuasha420 - That's a good point, however, I couldn't find a design for tablet for Subtle Notification. Feel free to create a separate issue for this, and we can ask Sigal for a tablet design.

@kelvinballoo If you do open an issue about this, please let it be a general enhancement issue for all subtle notifications, not specific to RRM. Thanks!

ITEM 6:
There is this part of the QAB which states : "Ensure both the primary and secondary CTAs dismiss the notification."
I understand that even clicking the CTAs would dismiss the notification.
I wanted us to review this because I felt like it was a bad experience personally.
My suggestion would be if we click on CTAs like 'Check publication status', it's not necessary to dismiss the notification?

We have changed this so that the banner is not dismissed anymore as part of the primary CTA.

Please let me know if you have any other concerns, thank you!

@nfmohit nfmohit assigned kelvinballoo and unassigned nfmohit Aug 20, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

Hi @nfmohit , @kuasha420 , this is tested good overall.
I have one item to highlight which is ITEM 4. The remaining were reviewed as good.

ITEM 4 ⚠️
Stacking is working great.
That said, personally, it will look better if the primary CTA (e.g. customize CTA) is at the top and 'Maybe later' CTA is at the bottom. It would be consistent with how the RRM dashboard banner is. Details are below.

I agree this is subjective and possibly it doesn't need to be done under this ticket. Maybe we can have a conversation with Sigal on this.

Current implementation is like this. Screenshot 2024-08-20 at 21 06 51

Compared to the RRM dashboard banner, the 'Maybe later' button is at the bottom:
Screenshot 2024-08-20 at 21 07 31

_______________________________________

ITEM 1
Colour of the icon is now correct at rgb(78, 51, 0) which is #4e3300

Screenshot 2024-08-20 at 20 54 51

ITEM 2
The state icon now appears on smaller viewports.

Screenshot 2024-08-20 at 20 57 22

ITEM 3
Noted on this.
Added a ticket here : https://app.asana.com/0/1207894970935603/1208105264866854


ITEM 5

Agreed. New ticket raised here: #9215


ITEM 6
Clicking the primary CTA will not dismiss the banner. Tested across all three variants.
'Maybe later' and 'Got it' buttons would dismiss the banner as expected.

primary.cta.ok.mov

@nfmohit
Copy link
Collaborator Author

nfmohit commented Aug 20, 2024

ITEM 4 ⚠️
Stacking is working great.
That said, personally, it will look better if the primary CTA (e.g. customize CTA) is at the top and 'Maybe later' CTA is at the bottom. It would be consistent with how the RRM dashboard banner is. Details are below.

I agree this is subjective and possibly it doesn't need to be done under this ticket. Maybe we can have a conversation with Sigal on this.

Thank you @kelvinballoo. I would suggest creating a new general improvement issue for this so that we can put this suggestion for all subtle notifications. Thank you.

@nfmohit nfmohit removed their assignment Aug 20, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Thanks for checking @nfmohit . This is good to go.
Moving to approval

ITEM 1
Colour of the icon is now correct at rgb(78, 51, 0) which is #4e3300

Screenshot 2024-08-20 at 20 54 51

ITEM 2
The state icon now appears on smaller viewports.

Screenshot 2024-08-20 at 20 57 22

ITEM 3
Noted on this.
Added a ticket here : https://app.asana.com/0/1207894970935603/1208105264866854


ITEM 4
Raised a new enhancement ticket under #9216


ITEM 5

Agreed. New ticket raised here: #9215


ITEM 6
Clicking the primary CTA will not dismiss the banner. Tested across all three variants.
'Maybe later' and 'Got it' buttons would dismiss the banner as expected.

primary.cta.ok.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Sp Wk 2 Issues to be completed in the second week of the assigned sprint Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants