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 setup CTA widget #8846

Closed
13 of 14 tasks
nfmohit opened this issue Jun 8, 2024 · 12 comments
Closed
13 of 14 tasks

Implement RRM setup CTA widget #8846

nfmohit opened this issue Jun 8, 2024 · 12 comments
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority 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 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.
  • The "Learn more" external link should open https://readerrevenue.withgoogle.com in a new tab (see comment in Figma).
  • 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 active.

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 active (isModuleActive selector), and it is not a view-only user, then display the CTA widget.

Test Coverage

  • Add tests for component.

QA Brief

  1. Deactivate the module. Make sure that feature is enabled.

  2. Go to Site Kit > Dashboard, banner must be visible.

  3. Ensure that design matches with the Figma design.

  4. Clicking on Set up Reader Revenue Manager should activate the module and redirect to setup module screen.

  5. Clicking on Maybe later should dismiss the banner permanently.

  6. Ensure that banner renders properly on tablet view and mobile view

  7. Also ensure that banner and graphic does not break when transitioning from tablet to desktop size.

  8. Ensure the banner is visible only on HTTPS-enabled sites.

Changelog entry

  • Add the Reader Revenue Manager module setup CTA banner notification to the Site Kit main dashboard, visible only on HTTPS-enabled sites.
@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 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 self-assigned this Jun 14, 2024
@nfmohit nfmohit changed the title Implement RRM banner notification Implement RRM setup CTA widget Jun 26, 2024
@nfmohit nfmohit removed their assignment Jun 26, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Jun 28, 2024
@ankitrox ankitrox assigned ankitrox and unassigned ankitrox Jun 28, 2024
@nfmohit nfmohit self-assigned this Jun 30, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jun 30, 2024

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 nfmohit assigned ankitrox and unassigned nfmohit Jun 30, 2024
@ankitrox
Copy link
Collaborator

ankitrox commented Jul 1, 2024

Thank you @nfmohit for reviewing the IB.

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

Thanks

@ankitrox ankitrox assigned nfmohit and unassigned ankitrox Jul 1, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jul 3, 2024

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 nfmohit assigned ankitrox and unassigned nfmohit Jul 3, 2024
@ankitrox
Copy link
Collaborator

ankitrox commented Jul 3, 2024

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 ankitrox assigned nfmohit and unassigned ankitrox Jul 3, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jul 3, 2024

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

@nfmohit nfmohit removed their assignment Jul 3, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jul 3, 2024

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

@eclarke1 eclarke1 removed the Next Up Issues to prioritize for definition label Jul 8, 2024
@ankitrox ankitrox assigned hussain-t and unassigned ankitrox Aug 2, 2024
@hussain-t hussain-t assigned ankitrox and unassigned hussain-t Aug 2, 2024
@ankitrox ankitrox assigned hussain-t and unassigned ankitrox Aug 6, 2024
@hussain-t hussain-t assigned ankitrox and unassigned hussain-t Aug 6, 2024
@ankitrox ankitrox assigned hussain-t and unassigned ankitrox Aug 7, 2024
@hussain-t hussain-t removed their assignment Aug 7, 2024
@hussain-t
Copy link
Collaborator

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
Copy link
Collaborator

hussain-t commented Aug 7, 2024

@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/.

@ankitrox ankitrox assigned hussain-t and unassigned ankitrox Aug 7, 2024
@hussain-t hussain-t assigned ankitrox and unassigned hussain-t Aug 7, 2024
@ankitrox ankitrox assigned hussain-t and unassigned ankitrox Aug 8, 2024
@hussain-t hussain-t removed their assignment Aug 8, 2024
@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Aug 13, 2024

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.

@ankitrox
Copy link
Collaborator

@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 ankitrox removed their assignment Aug 14, 2024
@kelvinballoo
Copy link
Collaborator

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 kelvinballoo removed their assignment Aug 15, 2024
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 Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants