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 settings view screen for RRM #8842

Closed
5 of 6 tasks
nfmohit opened this issue Jun 8, 2024 · 6 comments
Closed
5 of 6 tasks

Implement settings view screen for RRM #8842

nfmohit opened this issue Jun 8, 2024 · 6 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

The module settings view screen should be added for the Reader Revenue Manager module. The screen should simply show the ID of the connected publication.

Screenshot for reference

image


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

Acceptance criteria

  • The module settings view screen should be implemented for the Reader Revenue Manager module according to the Figma designs.
  • The screen should display the ID of the connected publication (labelled as "Publication"), followed by a notice that conveys the publication onboarding state (via the <PublicationOnboardingStateNotice> component being implemented by #8838).

Implementation Brief

  • In assets/js/modules/reader-revenue-manager/components/settings/SettingsView.js component added in Add JS entry point for RRM #8786, retrieve the publication ID with useSelect using getPublicationID selector.
  • Return a div container with googlesitekit-setup-module class name which should wrap the following element with the mentioned structure as below.
    • A wrapper div with the class name googlesitekit-settings-module__meta-items and inside it another divwith class googlesitekit-settings-module__meta-item.
    • The setting label should be an h5 with the class name googlesitekit-settings-module__meta-item-type and the setting value should be a p with the class name googlesitekit-settings-module__meta-item-data, that renders a DisplaySetting component with the value as publication ID.
    • A reference of assets/js/modules/analytics-4/components/settings/SettingsView.js can be taken to build in similar way.
  • Add the PublicationOnboardingStateNotice component to the main wrapper div.

Test coverage

  • Add story for the component.

QA Brief

Prerequisite

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

  • Ensure that you have publications set up in the publisher center.
  • Enable the rrmModule feature flag in the tester plugin.
  • Activate the Reader Revenue Manager module.
  • Go to Site Kit > Settings > Reader Revenue Manager.
  • Verify that the publication ID is displayed in the settings view screen.
  • Verify that the publication onboarding state notice is displayed below the publication ID for the ONBOARDING_ACTION_REQUIRED and PENDING_VERIFICATION onboarding states.
  • Note that the Publication label and the publication ID value font styles differ in the current implementation. The current implementation adheres to the other modules' settings view screen styles.

Changelog entry

  • Implement the settings view screen for the Reader Revenue Manager module.
@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
@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
@ankitrox ankitrox assigned ankitrox and unassigned ankitrox Jul 3, 2024
@nfmohit nfmohit self-assigned this Jul 4, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jul 6, 2024

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

  • Return a div container with googlesitekit-setup-module class name which should wrap following element.

An additional class name specific to the module would be nice to add, i.e. googlesitekit-setup-module--reader-revenue-manager.

  • A heading Publication and text which should be the publication ID. Note that the heading is smaller than the ID text. Please refer to the figma design. These both elements should be wrapped within a single div.

Using the appropriate class names and components in crucial here. Let's mention using the wrapper div to use the googlesitekit-settings-module__meta-item class, which should also have another wrapper div with the class googlesitekit-settings-module__meta-items.

The setting label should be an h5 with the class name googlesitekit-settings-module__meta-item-type and the setting value should be a p with the class name googlesitekit-settings-module__meta-item-data, that renders a DisplaySetting component with the publication ID.

It may also be beneficial to reference another module's SettingsView component to get an idea of the structure, e.g. assets/js/modules/analytics-4/components/settings/SettingsView.js. Using the appropriate markup is crucial to ensure a consistent structure.


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

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

ankitrox commented Jul 8, 2024

Thank you @nfmohit . I've updated the IB as per mentioned points.

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

nfmohit commented Jul 9, 2024

Thank you @ankitrox . I've made some minor linguistic improvements to the IB, it looks good to me 👍

IB ✅

@nfmohit nfmohit removed their assignment Jul 9, 2024
@hussain-t hussain-t self-assigned this Jul 28, 2024
@hussain-t hussain-t removed their assignment Aug 5, 2024
@techanvil techanvil self-assigned this Aug 5, 2024
techanvil added a commit that referenced this issue Aug 5, 2024
Enhancement/#8842 - Implement settings view screen for RRM
@techanvil techanvil removed their assignment Aug 5, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

This is good overall. I was able to run the script and the settings view screen looks like Figma with the correct styling to align with existing styling.

Screenshot 2024-08-06 at 11 37 20
__________________________________________________________________

I have 2 clarifications though:
CLARIFICATION 1: ⚠️
There are 3 variations of the settings view screen:

  1. Without the notice
  2. With the notice 'Your publication is still awaiting review. you can check its status in Reader Revenue Manager.'
  3. With the notice 'Your publication requires further setup in Reader Revenue Manager'

Currently, I can only get the 3rd variation.
Could you guide me on how to setup and review the 1st and 2nd variations please?

CLARIFICATION 2: ⚠️
This is out of scope for this ticket but checking in case we need to create another ticket or it's being handled in an existing ticket.
When I click on the '[See full details in Reader Revenue Manager]' link, I get routed to a page hitting 400 error.
The error page is attached.

Screenshot 2024-08-06 at 11 36 55

@kelvinballoo kelvinballoo assigned hussain-t and unassigned hussain-t Aug 6, 2024
@hussain-t
Copy link
Collaborator

Hi @kelvinballoo,

There are 3 variations of the settings view screen:

Please use the tester plugin to change the publication onboarding state. I have updated the QAB.

CLARIFICATION 2: ⚠️
This is out of scope for this ticket but checking in case we need to create another ticket or it's being handled in an existing ticket.
When I click on the '[See full details in Reader Revenue Manager]' link, I get routed to a page hitting 400 error.
The error page is attached.

This is a bug. However, as you said, it's out of the scope. Feel free to create a new ticket.

@kelvinballoo
Copy link
Collaborator

QA Update ✅

Thanks @hussain-t .

All 3 variations were verified and they are appearing accordingly:

  • Without the notice ✅

    Screenshot 2024-08-06 at 13 50 32
  • With the notice 'Your publication is still awaiting review. You can check its status in Reader Revenue Manager.' ✅

    Screenshot 2024-08-06 at 13 58 02
  • With the notice 'Your publication requires further setup in Reader Revenue Manager' ✅

    Screenshot 2024-08-06 at 11 37 20

As for the Clarification 2, I've raised another ticket separately here: #9144

As for this ticket, it's good to move to Approval.

@kelvinballoo kelvinballoo removed their assignment Aug 6, 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