site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard
verified publisher icon google

Implement RRM setup CTA widget

Open nfmohit opened this issue 1 year ago • 6 comments

Feature Description

A setup CTA widget should be added to the Site Kit main dashboard with an intention to raise awareness regarding Reader Revenue Manager. This widget will include a CTA to initiate the Reader Revenue Manager module setup and a dismissal to dismiss it persistently. This should only be shown if the site is on HTTPS.

Screenshot for reference

image

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

Acceptance criteria

  • A new setup CTA widget should be implemented for the Reader Revenue Manager module according to the Figma designs.
  • The Figma designs should be treated as the source of truth for the copy and graphic.
  • Clicking on the "Set up Reader Revenue Manager" CTA should navigate the user to the Reader Revenue Manager module setup.
  • Clicking on the "Maybe later" dismissal should dismiss the banner permanently.
  • It should be ensured that the mobile and tablet designs are adhered to.
  • The setup CTA widget should only appear in the Site Kit main dashboard if the rrmModule feature flag is enabled, it is not a view-only user, and the Reader Revenue Manager module is not connected.

Implementation Brief

  • [ ] Create a new component in assets/js/module/reader-revenue-manager/components/SetupCTABanner.js. This component should be exported with the withWidgetComponentProps utility that let's this component use the Widget and WidgetNull props. Refer assets/js/components/notifications/AdsModuleSetupCTAWidget.js as this component is pretty much similar.

    • [ ] Wrap the body in the Widget component adding the noPadding in order to correctly layout the SVGs later.
    • [ ] Use a Button component for the primary CTA with href attribute. We can make use of useActivateModuleCallback hook for the CTA by passing slug to it.
       // Pass `handleConnectModule` as `onClick` callback of CTA.
   const handleConnectModule = useActivateModuleCallback( 'reader-revenue-manager' );
    • [ ] Use a tertiary Button for the secondary CTA.
    • [ ] Export the three distinct SVGs from Figma for desktop, mobile and tablet, adding them to the SVG folder assets/svg/graphics/.
    • [ ] Import the new SVGs to render them within this component.
    • [ ] On clicking the "Maybe later", dismissItem from CORE_USER should be called and it should be dismissed permanently i.e. expiresInSeconds should be zero.
    • [ ] If the CTA banner is dismissed, return null from component. That means, do not render it.

Styles and Responsiveness

  • [ ] Create a new scss file: assets/sass/reader-revenue-manager/components/_googlesitekit-rrm-setup-cta.scss and use consistent class names for each styled Cell. Refer assets/js/components/notifications/AdsModuleSetupCTAWidget.js for all the necessary class names that have been used. This banner is similar in styling.
  • [ ] Apply font, and padding styles to each element of the widget to match Figma mock for desktop.
  • [ ] Import the mobile and tablet breakpoints in to the component:
const isMobileBreakpoint = breakpoint === BREAKPOINT_SMALL;
const isTabletBreakpoint = breakpoint === BREAKPOINT_TABLET;

Storybook

  • [ ] Create a new story file assets/js/reader-revenue-manager/components/SetupCTABanner.stories.js using the base story boilerplate loading the new SetupCTABanner component in it's default.

Displaying CTA Widget

  • [ ] In assets/js/components/DashboardMainApp.js, check if feature is enabled (useFeature hook), if RRM module is not connected (isModuleConnected selector), and it is not a view-only user, then display the CTA widget.

Test Coverage

  • Add tests for component.

QA Brief

Changelog entry

nfmohit avatar Jun 08 '24 12:06 nfmohit

Thank you for the IB, @ankitrox . Please take a look at my comments below:

  • Create a new boolean isSaving local component state with useState, default to false.

I don't see an instruction on where to use this local state. Also, I don't think this local state is necessary for this setup CTA widget.

  • Wrap the body in the Widget component adding the noPadding in order to correctly layout the SVGs later.

Since this component isn't technically a Site Kit widget that is rendered in a widget area, I'd suggest adding the point that this component should be exported with the withWidgetComponentProps utility that let's this component use the Widget and WidgetNull props.

  • Lay out the Widget body out using a Grid.
  • [ ] Create component title as a h3
  • [ ] Create the description as a p.

Mentioning the reusable class names is crucial here, e.g. googlesitekit-setup-cta-banner, googlesitekit-setup-cta-banner__primary-cell, googlesitekit-setup-cta-banner__title, googlesitekit-setup-cta-banner__description, etc, as these class names provide the necessary styles. It might just be simpler to mention using assets/js/components/notifications/AdsModuleSetupCTAWidget.js as a reference point so that the structure is easier to replicate.

  • Use a Button component for the primary CTA with href attribute. We can make use of getAdminReauthURL selector.
   select('modules/reader-revenue-manager').getAdminReauthURL()

The button component should call the useActivateModuleCallback hook function on click with the module slug to navigate the user to the module setup. This will simply redirect the user, so it isn't necessary to have an in progress/saving state.

  • On clicking the "Maybe later", dismissPrompt from CORE_USER should be called and it should be dismissed permanently i.e. expiresInSeconds should be zero.

The dismissed-prompts infrastructure should only be used when it is necessary to dismiss something multiple times at different durations. As this is a permanent dismissal, we should use the dismissItem action instead.

Styles and Responsiveness

  • [ ] Create a new scss file: assets/sass/reader-revenue-manager/components/_googlesitekit-rrm-setup-cta.scss and use consistent class names for each styled Cell.
  • The added googlesitekit-setup-cta-banner and related reusable class names should apply most of the styles, as this is very similar to other setup CTA widgets.
  • It isn't necessary to explicitly mention each styling element, apart from replicating the Figma designs in the added stylesheet.
  • Any instructions regarding responsive designs should follow the mobile-first principle, so that the mobile styles are implemented by default, and overwritten for bigger viewpoints.

Storybook

  • [ ] Create a new story file assets/js/reader-revenue-manager/components/SetupCTABanner.stories.js using the base story boilerplate loading the new SetupCTABanner component in it's default (not isSaving state).

Since there is no isSaving state needed, let's remove its reference.

Test Coverage

  • Add tests for component. Check different dismissal scenarios.

There is only one dismissal scenario, as far as I can see. Just basic test coverage for this component should be sufficient.

I've added an additional AC regarding rendering the setup CTA widget. Please add instructions to render the component in the main dashboard, and only when the feature flag is on.

Please let me know if you have any questions, thank you!

nfmohit avatar Jun 30 '24 14:06 nfmohit

Thank you @nfmohit for reviewing the IB.

I have made the changes as per suggestions. Assigning back to you for re-review.

Thanks

ankitrox avatar Jul 01 '24 05:07 ankitrox

Thank you for updating the IB, @ankitrox !

I'd suggest adding the reference to assets/js/components/notifications/AdsModuleSetupCTAWidget.js upwards in the component description for SetupCTABanner.js, as that is a more suitable place for the reference to use the class names. In that way, there isn't much necessity to explicitly specify what elements to use (e.g. h3, p) as both the banners are quite similar.

We also don't need to specify the specific CSS styles in different breakpoints. Simply mentioning that the layout should follow the Figma designs should be sufficient. The IB doesn't need to be such verbose.

Please let me know what you think, thank you!

nfmohit avatar Jul 03 '24 08:07 nfmohit

Thank you @nfmohit. I have updated IB.

Moved the component reference to the first point and removed unnecessary styling instructions as those can be referred from the AdsModuleSetupCTAWidget component.

ankitrox avatar Jul 03 '24 12:07 ankitrox

Much better now, thank you @ankitrox ! IB LGTM 👍 ✅

nfmohit avatar Jul 03 '24 14:07 nfmohit

Note: Added a point in the AC and IB to not display the banner for a view-only user.

nfmohit avatar Jul 03 '24 17:07 nfmohit

Note: I have updated the ACs to include a target location for the "Learn more" link. It has been set according to this comment in Figma.

nfmohit avatar Jul 29 '24 17:07 nfmohit

Hi @ankitrox, as discussed on a call, I'm moving this issue back to execution to move the ReaderRevenueManagerSetupCTABanner component, its storybook file, and test file to the assets/js/modules/reader-revenue-manager/components/dashboard directory.

hussain-t avatar Aug 07 '24 12:08 hussain-t

~@nfmohit, should we navigate the user to https://publishercenter.google.com/ instead of https://readerrevenue.withgoogle.com/ when clicking the CTAs? cc: @ankitrox~

Apologies, it should be https://readerrevenue.withgoogle.com/.

hussain-t avatar Aug 07 '24 12:08 hussain-t

QA Update ⚠️

Hi @ankitrox , I have tested this and while I have found some discrepancies in Figma, these are minor and since we are not doing pixel-perfect, I am not including those here.

But I have 2 clarifications: CLARIFICATION 1: The user would see the banner to help set up RRM. After we have set up RRM and we disconnected the module. Should we be seeing the banner again on the dashboard? Currently, it will appear again.

CLARIFICATION 2: Quoting from the AC: The setup CTA widget should only appear in the Site Kit main dashboard if the rrmModule feature flag is enabled, it is not a view-only user, and the Reader Revenue Manager module is not active.

I did not really understand the second part : " it is not a view-only user, and the Reader Revenue Manager module is not active." Could you elaborate on this please.

kelvinballoo avatar Aug 13 '24 12:08 kelvinballoo

@kelvinballoo

CLARIFICATION 1:

This is expected behaviour and it is same for all other CTA widget components. I believe, the banner was not dismissed previously in this case.

CLARIFICATION 2:

Banner should only appear if ALL of the following conditions are met.

  1. Feature is enabled.
  2. Current user is admin (not view-only user).
  3. Module is not activated.

Please let me know if you have any other queries.

Thanks

ankitrox avatar Aug 14 '24 15:08 ankitrox

QA Update ✅

Thanks for the clarifications @ankitrox . Noted that it's as expected for item 1 and understood item 2 as well.

This has been verified good as follows. Moving ticket to approval.

  • The banner appears when

    • Feature is enabled.
    • Current user is admin (not view-only user).
    • Module is not activated. Turn off the RRM flag and ensure banner doesn't appear. Activating the module will make the banner disappear. ✅

  • Banner matches with the Figma design. There are some discrepancies with Figma but nothing too drastic since we are not striving to be pixel-perfect ✅

    Screenshot 2024-08-15 at 17 35 23

  • Looking good on mobile and tablet ✅

    Screenshot 2024-08-12 at 17 22 13 Screenshot 2024-08-15 at 17 43 57

  • The banner is not visible only on HTTP sites. ✅

    Screenshot 2024-08-12 at 18 27 52

  • Clicking on Set up Reader Revenue Manager activates the module and redirects to setup module screen. ✅ Clicking on Maybe later should dismiss the banner permanently.

    https://github.com/user-attachments/assets/d4fdc2d0-e070-4cff-a84d-6daa1d2d407b

kelvinballoo avatar Aug 15 '24 13:08 kelvinballoo