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 <PublicationOnboardingStateNotice> component #8838

Closed
9 tasks done
nfmohit opened this issue Jun 8, 2024 · 9 comments
Closed
9 tasks done

Implement <PublicationOnboardingStateNotice> component #8838

nfmohit opened this issue Jun 8, 2024 · 9 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 <PublicationOnboardingStateNotice> component should be implemented for the Reader Revenue Manager module that renders a notice based on the onboarding state of the current publication.

Screenshot for reference

image


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

Acceptance criteria

  • A <PublicationOnboardingStateNotice> component should be added to the Reader Revenue Manager module according to the Figma designs that renders a notice based on the onboarding state of the current publication.
  • If the current/selected publication is awaiting review or needs additional steps, that should be conveyed to the user via this notice, otherwise, it should output nothing.
  • If the publication is awaiting review:
    • This can be determined if the onboarding state of the publication is PENDING_VERIFICATION.
    • Text: "Your publication is still awaiting review. you can check its status in Reader Revenue Manager."
    • CTA: "Check publication status" (with external link icon). This should open the publication in the publisher center in a new browser tab.
  • If the publication needs additional steps:
    • This can be determined if the onboarding state of the publication is ONBOARDING_ACTION_REQUIRED.
    • Text: "Your publication requires further setup in Reader Revenue Manager"
    • CTA: "Complete publication setup" (with external link icon). This should open the publication in the publisher center in a new browser tab.

Implementation Brief

Note: PropertySelect component in #8837 will be responsinble for setting the onboarding state in the settings using the setPublicationOnboardingState action. This has already described in IB for #8837.

  • Create a new component in assets/js/module/reader-revenue-manager/components/common/PublicationOnboardingStateNotice.
    • Component should make use of useSelect hook to check for the publicationOnboardingState and call getPublicationOnboardingState selector.
    • If the value of the onboarding is not PENDING_VERIFICATION or ONBOARDING_ACTION_REQUIRED, return null.
    • If the onboatding state is within one of PENDING_VERIFICATION or ONBOARDING_ACTION_REQUIRED, display the notice text and CTA text as per AC mentioned above.
    • Use the SettingsNotice component to display the notice. Pass following props to it.
      • Icon - Pass warning as we need to display the warning notice.
      • notice - This will be the notice text as per AC.
      • OuterCTA - This should be CTA which will open the publication center (https://publishercenter.google.com/) in a new tab. Use Link component to display CTA link with external prop set to true. We should use getServiceURL selector to get the URL for the CTA. Pass publication ID to the selector.
  • Refer the figma design for PublicationOnboardingStateNotice component to match the notice component with the design.

Test Coverage

Add tests for component with different onboarding states.

QA Brief

  • Verify the Modules -> ReaderRevenueManager -> Common -> PublicationOnboardingStateNotice Storybook story and verify that the notice looks according to the Figma designs and functions according to the ACs.
  • This notice is rendered in the RRM module setup and settings screens when the selected publication does not have its onboarding completed. Verify that the notice appears correctly according to the Figma designs and acts according to the ACs.
    • If your selected publication already has its onboarding completed, or you do not have such a publication, this can be mocked by using the following script in the browser console:
     googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).setPublicationOnboardingState( "PENDING_VERIFICATION" );
     // You can also try `ONBOARDING_ACTION_REQUIRED` instead of `PENDING_VERIFICATION`.
    • Note: Some of the spacings around the notice may not exactly match the Figma designs. This is expected and is consistent with reusable components used across other modules.

Changelog entry

  • Add the PublicationOnboardingStateNotice component, rendering a notice based on the onboarding state of the current publication and presented in Storybook.
@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
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jun 18, 2024
@ankitrox ankitrox assigned ankitrox and unassigned ankitrox Jun 26, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Jun 27, 2024
@nfmohit nfmohit self-assigned this Jun 27, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jun 30, 2024

Thank you for the excellent IB here, @ankitrox !

One small point about the CTA link. We should use the getServiceURL selector with the current publication ID passed to get the URL for the CTA. Could you please update it?

Thanks!

@nfmohit nfmohit assigned ankitrox and unassigned nfmohit Jun 30, 2024
@ankitrox
Copy link
Collaborator

ankitrox commented Jul 1, 2024

@nfmohit Thank you for reviewing the IB.

I've amended the suggested point. Assigning back to you.

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

nfmohit commented Jul 3, 2024

Thanks @ankitrox ! IB LGTM 👍 ✅

@nfmohit nfmohit removed their assignment Jul 3, 2024
@eclarke1 eclarke1 removed the Next Up Issues to prioritize for definition label Jul 8, 2024
@ankitrox ankitrox self-assigned this Jul 16, 2024
@techanvil techanvil assigned ankitrox and unassigned techanvil Jul 30, 2024
@ankitrox ankitrox assigned techanvil and unassigned ankitrox Jul 31, 2024
@techanvil techanvil removed their assignment Jul 31, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

Couple of things to flag.
I understand that some of these could arise due to us trying to keep things consistent across the whole plugin and if that's the case, that's fine and these can be ignored.

ITEM 1:
Colour of the icon with the exclamation mark is almost dark. It should have been brown at #4E3300
Also, the icon size is implemented as 19x19. It should have been 22x 22

Screenshot 2024-08-02 at 15 34 25

Icon size:
Screenshot 2024-08-02 at 15 32 40

Figma icon size and colour:
Screenshot 2024-08-02 at 15 32 20


ITEM 2:

I could not verify the font weight of the CTA text "Your publication is still awaiting review. You can check its status in Reader Revenue Manager." as I could not see the attribute in dev tools.
It feels like it's 400 though.

Based on Figma, it's supposed to be 500px

Screenshot 2024-08-02 at 16 03 10

ITEM 3:
Marins around the button is supposed to be 18px top and bottom and 24px on the side.
Currently, it's 16px on all sides.

**Figma:** Screenshot 2024-08-02 at 15 38 35

Implementation:
Screenshot 2024-08-02 at 15 37 15

@ankitrox
Copy link
Collaborator

ankitrox commented Aug 5, 2024

As per the conversation in this slack thread, this issue will be on hold in Execution till #8840 is merged so that we can use SubtleNotification instead of SettingsNotice and address other styling concerns that it may have.

CC: @kelvinballoo @wpdarren

@tofumatt
Copy link
Collaborator

tofumatt commented Aug 6, 2024

Just a note here, that the existing work merged in (see PR #9037) was entirely self-contained and only affected Storybook/Jest tests; the actual component is not yet in use anywhere and there are no other changes.

To simplify things I'm going to remove it from being tagged with the current release, since it's dependent on another issue (#8840), which won't be ready in time, and this issue will not be merged into this release (1.133.0).

Normally we'd revert this change from develop/main, but in this case we can simplify untag it. In the future please keep in mind issues that are tagged with the release should be removed if they won't be completed for the release 🙂

@ankitrox
Copy link
Collaborator

ankitrox commented Aug 8, 2024

As discussed previously, I have created a follow-up PR #9177 for this issue which uses SubtleNotification component. Also, there are couple of changes in SubtleNotification component to allow a notification without dismiss CTA and added related story for it.

Moving this for code review.

@ankitrox ankitrox removed their assignment Aug 8, 2024
@nfmohit nfmohit assigned nfmohit and ankitrox and unassigned nfmohit Aug 8, 2024
@ankitrox ankitrox assigned nfmohit and unassigned ankitrox Aug 9, 2024
@nfmohit nfmohit assigned ankitrox and unassigned nfmohit Aug 20, 2024
@ankitrox ankitrox assigned nfmohit and unassigned ankitrox Aug 21, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Aug 22, 2024

Hi @kelvinballoo . This is now back with you for a re-check.

ITEM 1:
Colour of the icon with the exclamation mark is almost dark. It should have been brown at #4E3300
Also, the icon size is implemented as 19x19. It should have been 22x 22

This has been fixed.

ITEM 2:

I could not verify the font weight of the CTA text "Your publication is still awaiting review. You can check its status in Reader Revenue Manager." as I could not see the attribute in dev tools. It feels like it's 400 though.

Based on Figma, it's supposed to be 500px

It is 500.

Screenshot

image

ITEM 3:
Marins around the button is supposed to be 18px top and bottom and 24px on the side.
Currently, it's 16px on all sides.

This has been fixed.

Please let us know if you have any other questions, thank you!

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

QA Update ✅

Thanks @nfmohit ,

Verified this and it's looking good.

ITEM 1:
Icon size is roughly 24px which is same as figma and the icon now has a brown background colour.

Screenshot 2024-08-26 at 20 58 24 Screenshot 2024-08-26 at 20 59 09
____________________________________________________

ITEM 2:
Noted on that.


ITEM 3:
I can confirm the button margins/paddings are now 18px top and bottom and 24px on the sides.

Screenshot 2024-08-26 at 21 00 15

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

9 participants