google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 286 forks source link

Implement `<PublicationSelect>` component #8837

Closed nfmohit closed 2 months ago

nfmohit commented 4 months ago

Feature Description

A <PublicationSelect> component should be added for the Reader Revenue Manager module that presents the user with a list of available publications and allows them to select one of them. The selected publication should be stored in the data store within the publicationID module setting, with its onboarding state stored within the publicationOnboardingState module setting.

Screenshot for reference ![image](https://github.com/google/site-kit-wp/assets/20284937/26726198-52c8-4bf7-90c6-5e55e790cd85)

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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Note 1: Ensure that you have publications setup in publisher center. Refer QAB of this issue on how to setup publications. Also, there should be atleast two publications setup for testing this.

Note 2: Ensure that "Custom Site URL" is NOT set in "Tester Settings" (Site Kit by Google Tester plugin).

  1. Go to Site Kit > Settings > Connecte More Services > Reader Revenue Manager, click Set up Reader Revenue Manager CTA.

  2. This will take you to the setup screen where initially there must be a loading progress bar till publications are getting fetched.

  3. Once publications are fetched, select component will be visible.

  4. Run the following command in browser console.

  googlesitekit.data.select('modules/reader-revenue-manager').getSettings();

which should produce the following output.

    {
      "publicationID": "",
      "publicationOnboardingState": "",
      "publicationOnboardingStateLastSyncedAtMs": 0
    }
  1. Select any publication from the list and run the same command. This time there must be publicationID and publicationOnboardingState set in the response.

  2. Repeat step 5 by selecting other publications, note that every time the publication ID needs to be different.

Changelog entry

nfmohit commented 3 months ago

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

  • Update the getPublications selector to also retrieve onboardingState in publication object. This should already be received from GET:publications endpoint.

Let's remove this requirement as this property should already be a part of the getPublications selector. See this comment.


I think it might be beneficial to use the Analytics PropertySelect component as a starting point. See points below:

  1. Let's mention the usage of the isDisabled and hasModuleAccess props. The getPublications selector should only be called if hasModuleAccess isn't explicitly false, and isDisabled isn't falsy, similar to the Analytics PropertySelect component.
  2. Let's add validation for the publication ID, i.e. isValidPublicationID function. This should be similar to the Analytics isValidMeasurementID function which checks that the value is a string which only contains alphabets and numbers.
  3. Let's mention that we want to render a Select component with no publications (other than if one is already selected) if hasModuleAccess is explicitly false, similar to the Analytics PropertySelect component.
  4. Let's mention that we want to display a loading progress bar when the publications are being resolved, again, similar to the Analytics PropertySelect component.

For Storybook stories, I don't think it'll be very valuable to add a Storybook story for this component alone. This will likely be included in the Storybook stories for the module setup and settings screens, so an individual story may seem redundant.


Let me know what you think and if you have any questions, thank you!

ankitrox commented 3 months ago

Thanks @nfmohit for reviewing the IB.

I have updated the IB as per the suggested points. Re-assigning to you for review.

Thanks again!

nfmohit commented 3 months ago

Thank you for the updates, @ankitrox ! I've made a few changes to the IB to reduce the back and forth, namely:

  1. I've mentioned using Analytics PropertySelect as a reference point.
  2. I've removed the proposal to update the getPublications selector for onboarding state, as that should be no longer required.
  3. I made the criteria for isValidPublicationID a little more specific and mentioned that it should only accept alphabets and digits (no symbols).
  4. I've mentioned that we can use the isResolving selector for getPublications to determine the loading state, instead of proposing to create a new selector. Working on a new selector unless necessary also includes adding test coverage for it, thus requiring more time and effort.

Please let me know if you have any questions. IB LGTM 👍 ✅

kelvinballoo commented 2 months ago

QA Update ⚠️

Few items to raise: ITEM 1: Copy The text description is currently 'This is just added as a placeholder component to assist with testing.' Based on figma, it should be 'Select your preferred publication to connect with Site Kit'. Is this going to be tackled under a different issue?

Implementation: Screenshot 2024-07-24 at 11 14 48 Figma: Screenshot 2024-07-24 at 14 31 52

ITEM 2: Publication ID Missing and dropdown size The dropdown should have included the Publication ID as well. Currently, it doesn't. Moreover, the size of the dropdown should be 300px wide, not 200px.

Implementation: Dropdown is 200px wide Screenshot 2024-07-24 at 14 32 13 Figma size is 300px: Screenshot 2024-07-24 at 14 31 52

ITEM 3: Value for publicationOnboardingStateLastSyncedAtMs The "publicationOnboardingStateLastSyncedAtMs" is always 0 regardless of the publication selected. It's also 0 after some time that I've selected the publication at the dropdown. Are we expecting any values besides 0?

Screenshot 2024-07-24 at 14 29 19

ITEM 4: Spacing From the icon to the description text, it's supposed to be around 45px. Currently, it's just 14px.

Figma: Screenshot 2024-07-24 at 14 20 55 Implementation: Screenshot 2024-07-24 at 14 55 25
ankitrox commented 2 months ago

ITEM 1: Copy

As you can see from the discussion on previous PR, we just added the component along with the message so that it is convenience to test. We will test it thoroughly in #8800

ITEM 2: Publication ID Missing and dropdown size

Sorry for not including this. It looks like this was missed during our previous round. I have created a new PR #9073 to address the issue. Also, addressed the width issue in same PR, although I believe that we could have kept the width as is to maintain consistency along with other select components. We can check if there is any feedback in CR for the same.

ITEM 3: Value for publicationOnboardingStateLastSyncedAtMs

This is will be handled in #8796 here . The reason, it can't be seen in action as of now is because we are yet to integrate everything in setup screen. We can test this in #8800

ITEM 4: Spacing

As per the discussion mentioned previously, we can test all the styling things in #8800

techanvil commented 2 months ago

Back to you for another pass, @kelvinballoo.

Please note that with regard to Item 2, the dropdown size is derived from content. As per this https://github.com/google/site-kit-wp/pull/9073#discussion_r1691287607 (which you may have already seen):

... the Select dropdown takes its width from its content, and this should be understood to be reflected in the Figma design rather than a fixed minimum width.

kelvinballoo commented 2 months ago

QA update ✅

Noted on Item 1, 3 and 4. Tested item 2 and the dropdown is now reflecting the Publication ID accordingly, similar to the Publisher Central.

Screenshot 2024-07-26 at 11 33 52

Also tested the following:

Moving ticket to Approval.

techanvil commented 2 months ago

Moving this back to execution to fix the RRM SetupMain story that developed an error as a result of its first PR.

aaemnnosttv commented 2 months ago

Thanks @techanvil , merged 👍