site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Implement `<PublicationOnboardingStateNotice>` component

Open nfmohit opened this issue 1 year ago • 3 comments

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

Changelog entry

nfmohit avatar Jun 08 '24 10:06 nfmohit

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 avatar Jun 30 '24 02:06 nfmohit

@nfmohit Thank you for reviewing the IB.

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

ankitrox avatar Jul 01 '24 04:07 ankitrox

Thanks @ankitrox ! IB LGTM 👍 ✅

nfmohit avatar Jul 03 '24 08:07 nfmohit

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

kelvinballoo avatar Aug 02 '24 12:08 kelvinballoo

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

ankitrox avatar Aug 05 '24 05:08 ankitrox

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 🙂

tofumatt avatar Aug 06 '24 22:08 tofumatt

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 avatar Aug 08 '24 12:08 ankitrox

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 avatar Aug 22 '24 00:08 nfmohit

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 avatar Aug 26 '24 17:08 kelvinballoo