google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.22k stars 279 forks source link

Implement RRM module setup success banner #8840

Closed nfmohit closed 3 days ago

nfmohit commented 2 months ago

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](https://github.com/google/site-kit-wp/assets/20284937/38940ecb-848a-4c8d-b4ee-e799dfca00ac) User needs to perform additional steps to complete publication setup: ![image](https://github.com/google/site-kit-wp/assets/20284937/7ab65018-ced3-41db-aeff-ed56fecafc82) Publication setup is complete: ![image](https://github.com/google/site-kit-wp/assets/20284937/791fac64-2015-4814-b5c2-7858ffebcca7)

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

Acceptance criteria

Implementation Brief

Reusable SubtleNotification component

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

RRMSetupSuccessSubtleNotification component

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

Add the notification to the Dashboard

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

Test Coverage

QA Brief

Prerequisite

Changelog entry

nfmohit commented 1 month ago

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!

hussain-t commented 1 month ago

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.

nfmohit commented 1 month ago

Thanks @hussain-t ! IB LGTM πŸ‘ βœ…

hussain-t commented 1 month ago

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

nfmohit commented 2 weeks ago

@kuasha420 Just wanted to share a few observations that I noticed while reviewing another issue that uses the SubtleNotification component extracted here:

  1. The color of the info icon in the warning state is not correct. Figma: image Implementation: image
  2. The state icon disappears in smaller viewports. Figma: image Implementation: image

CC: @hussain-t

hussain-t commented 2 weeks ago

Thanks for spotting this, @nfmohit.

The state icon disappears in smaller viewports.

That’s the default behavior of the Subtle Notification implementation. We should review the other Subtle Notification designs in Figma to check if the icon is visible in smaller viewports. I think we should ensure consistency across all notifications, whether the icon is visible or not.

nfmohit commented 2 weeks ago

Sounds good, thank you @hussain-t !

@kuasha420 - One additional observation (as @wpdarren pointed out here):

This setup success banner should also prevent the display of the original module setup success banner. I think this could've been mentioned explicitly in the ACs.

nfmohit commented 1 week ago

Thanks for spotting this, @nfmohit.

The state icon disappears in smaller viewports.

That’s the default behavior of the Subtle Notification implementation. We should review the other Subtle Notification designs in Figma to check if the icon is visible in smaller viewports. I think we should ensure consistency across all notifications, whether the icon is visible or not.

@hussain-t On this topic, I also checked the Figma designs for the Subtle Notification in Audience Segmentation, and that does seem to include the icon in smaller viewports. See:

image

It looks like we do not have mobile designs for GA4AdSenseLinkedNotification, PAXSetupSuccessSubtleNotification, or Ads SetupSuccessSubtleNotification.

Based on my observation, it looks like we should be keeping the icon for the smaller viewports. What do you think?

hussain-t commented 1 week ago

Thanks for confirming, @nfmohit. SGTM πŸ‘

kelvinballoo commented 1 week ago

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.

https://github.com/user-attachments/assets/5e32de70-db15-49de-93b9-b237ccc50a1b

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:

https://github.com/user-attachments/assets/7b99cfbe-e71a-44e5-9b28-afe5f9bd50da
kuasha420 commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

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 commented 6 days ago

Top stuff, thanks @nfmohit.

nfmohit commented 6 days ago

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!

kelvinballoo commented 6 days ago

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: https://github.com/google/site-kit-wp/issues/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.

https://github.com/user-attachments/assets/9d4d4b3c-e7cd-4645-b86d-696121e1e919
nfmohit commented 6 days ago

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.

kelvinballoo commented 6 days ago

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 https://github.com/google/site-kit-wp/issues/9216


ITEM 5 βœ…

Agreed. New ticket raised here: https://github.com/google/site-kit-wp/issues/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.

https://github.com/user-attachments/assets/9d4d4b3c-e7cd-4645-b86d-696121e1e919