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

Add the Settings Section setup CTA variant including happy path setup behavior

Open techanvil opened this issue 1 year ago • 7 comments

Feature Description

Add the Settings Section setup CTA variant including happy path setup behavior.

This includes the setup logic (which may be extracted for reuse from the Setup CTA Banner if not already reusable) including OAuth redirection, the in progress state and the success state.

Note that the success state can reuse the component introduced in https://github.com/google/site-kit-wp/issues/8238.

See Settings section > setup CTA in the design doc.


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

Acceptance criteria

  • If the Audience Segmentation feature has not been set up (i.e. the Setup CTA Banner has not been actioned), the Settings section should display a setup CTA instead of the switch to toggle the audiences widget area.
  • The setup CTA should follow the Figma design.
  • It should have a description with the following copy: To set up new visitor groups for your site, Site Kit needs to update your Google Analytics property.
  • It should have an Enable groups link button.
  • Upon clicking Enable groups, the feature will be set up following the setup logic described for the Setup CTA Banner (including the OAuth flow, if the edit scope is not granted. See #8131 and #8132), while showing a progress bar for the in progress state (see Figma).
  • Upon success, a dismissible notice will be displayed alongside the switch for toggling the audiences widget area (see Figma).
    • It should have the following copy: We've added the audiences section to your dashboard!
    • It should have Show me and Got it primary and secondary CTAs.
    • Clicking Show me should navigate to the dashboard and scroll down to the newly added widget area.
    • Clicking Got it should dismiss the notice.
  • Note that all copy should be verified with Figma as the source of truth.

Implementation Brief

This should be implemented once #8132 is implemented and merged.

  • [ ] Create a hook useAudienceSegmentationSetup inside assets/js/modules/analytics-4/hooks/.

    • [ ] useAudienceSegmentationSetup should return an object consisting of the following properties.
      • [ ] isSaving - This is a boolean flag indicating whether the setup operation is in progress. This can be constructed using the useState hook and will be set inside onEnableGroups callback as it is currently in AudienceSegmentationSetupCTAWidget component.
      • [ ] onEnableGroups - Callback for Enable groups CTA. This should be taken as is from AudienceSegmentationSetupCTAWidget component from here with all the dependecies for the useCallback.
      • [ ] canRender - Return null from the component if this is false, else render the component. Refer this condition under which this can be evaluated.
        • [ ] Hook should also call this useEffect as this will be applicable to both the components and any other similar component.
  • [ ] Create a new component SettingsVisitorGroupsCTA inside assets/js/modules/analytics-4/components/audience-segmentation/settings/.

    • [ ] We can take a reference of SettingsCardVisitorGroups component to create the UI for SettingsVisitorGroupsCTA component, its similar.
    • [ ] Component should use useAudienceSegmentationSetup hoook which will return the object as mentioned above.
      • [ ] If isSaving is false render the Enable groups CTA, else render the ProgressBar component. We may need to add a fix height when displaying the ProgressBar to avoid the layout shift (~142px).
      • [ ] onClick callback of CTA should be onEnableGroups callback returned by the hook.
  • [ ] Create a new component AudienceSetupSuccessSettingsNotice inside /assets/js/modules/analytics-4/components/audience-segmentation/settings/ which should be responsible for displaying the success notice within SettingsCardVisitorGroups component.

    • [ ] Item key for component to display the notice can be audience_segmentation_setup_settings_notification.
  • [ ] Update SettingsCardVisitorGroups component to display the success message as per following.

    • [ ] Check if the success banner has already been dismissed. Refer AudienceSegmentationSetupSuccessSubtleNotification component for the item key and how it handles the banner here. Note that the item key needs to be different as mentioned above so that relevant notice is being displayed.
    • [ ] Use dismiss item infra from core/user data store to check if the item is dismissed and to also dismiss the item.
    • [ ] Success banner should be displayed in SettingsCardVisitorGroups component if it has not been dismissed earlier.
    • [ ] Clicking the Got it should dismiss the notice.
    • [ ] Clicking show me should do the following.
      • [ ] Dismiss the notice.
      • [ ] Use navigateTo function to navigate to the audience widget area.
  • [ ] Update the AudienceSegmentationSetupCTAWidget to use useAudienceSegmentationSetup hook and remove the code which has been ported to hook.

Test coverage

  1. Add the stories for SettingsVisitorGroupsCTA component with all the states.
  2. Add the test for the SettingsVisitorGroupsCTA component with different states and props.
  3. Fix the story for AudienceSegmentationSetupCTAWidget component to accommodate changes.
  4. Fix any failing tests and VRTs.
  5. Add tests for the useAudienceSegmentationSetup hook. 6 . Update the SettingsCardVisitorGroups component tests to add tests for the success notice behaviour.
  6. Add tests for AudienceSetupSuccessSettingsNotice component.

QA Brief

Changelog entry

techanvil avatar Jan 25 '24 12:01 techanvil

Thanks @ankitrox for drafting the IB. I've given an initial pass over the IB and the approach looks solid. A few questions and suggestions:

  • The useAudienceSegmentationSetup should probably reside in assets/js/modules/analytics-4/hooks as the feature is analytics module specific and it will be used here.
  • The hook also should have basic test coverage if possible.
  • It would be good to detail a little bit more about the hook and what it should do, or how the suggested values should be obtained before being returned.

Let me know what you think, cheers!

kuasha420 avatar Jun 06 '24 18:06 kuasha420

Thank you so much @kuasha420 for reviewing the IB.

I have made some amendments in IB as per your suggestions. Assigning this to you for re-review.

Thanks again!

ankitrox avatar Jun 07 '24 07:06 ankitrox

Thanks @ankitrox, Looking good now. However, I have missed a point in my previous review. The second to last point of the AC states "Upon success, a dismissible notice will be displayed alongside the switch for toggling the audiences widget area", this is not covered in current IB. Can you add that,

Besides that, it would be nice to add a pointer/note about trying to avoid layout shift while the progressbar is showing by using a appropriate height for the Visitor Group area and other relevant component. What do you think?

Cheers!

kuasha420 avatar Jun 10 '24 03:06 kuasha420

@kuasha420 Thanks for the spotting the missing point about the success notice.

I have addressed the feedback. assigning this to you for re-review.

Thanks

ankitrox avatar Jun 10 '24 16:06 ankitrox

Thanks @ankitrox for the additions to the IB.

It is worth pointing out that the success notice for settings should use a different dismiss item key, Otherwise, if someone activates the Audience Group in Dashboard, and visits settings without dismissing the success notification, they'll see it above the toggle here as well which may not be desirable behavior.

Also, this should be a new component that renders inside the SettingsCardVisitorGroups, ie. AudienceSetupSuccessSettingsNotice which is similar in looks to the AudienceSegmentationSetupSuccessSubtleNotification.

Cheers.

kuasha420 avatar Jun 11 '24 06:06 kuasha420

Thank you @kuasha420 .

Updated the IB to include new component AudienceSetupSuccessSettingsNotice to display success notice and also included mention about the different item key.

ankitrox avatar Jun 12 '24 05:06 ankitrox

Thanks @ankitrox. LGTM. I've bumped up the estimate one level, considering this kind of issues are generally more involved that initially feels, and the extensive test coverage suggestion.

IB :white_check_mark:

kuasha420 avatar Jun 13 '24 14:06 kuasha420

Hi @techanvil . The ACs for this issue include a requirement to scroll down to the Audience Tiles widget area. However, this functionality is planned to be built as part of #8874. Should the AC for #8874 be updated to include updating the functionality for the settings setup success notification as well? Thanks!

nfmohit avatar Jul 09 '24 08:07 nfmohit

Hi @nfmohit, personally I would prefer to keep the AC as they stand, as the IB for #8874 has already been approved and it would need a rewrite; the common code between this and the behaviour in #8874 is just a call to getContextScrollTop() and scrollTo(), the triggers will be completely different.

It will also be natural to test the scrolling as part of this issue, as it's part and parcel of the Settings flow that is being implemented. The main reason #8874 was created separately is that the widget area wasn't available to scroll to when the setup happy path was first implemented, which isn't the case here. If this issue was larger then it might be worth splitting it out, but as it's currently scoped and estimated I think it's fine to keep it here. Of course, if the issue was underestimated and looks like it's going to go much over the estimate it could be worth reconsidering.

techanvil avatar Jul 09 '24 09:07 techanvil

@techanvil Co-assigned you on this as we request your kind feedback here, thank you!

nfmohit avatar Jul 16 '24 07:07 nfmohit

QA Update ⚠️

I only 2 feedback:

ITEM 1: First item is regarding the fonts size and weight used. While it's reflecting as per figma, it's not consistent with other text on the page.

  • The 'Enable groups' is at font weight 500 but other similar CTA on the page are at 400.
  • The text 'To set up new visitor groups for your site, Site Kit needs to update your Google Analytics property.' is currently at 12px, similar to Figma but other similar text on the page are at 14px.

Refer to the video for more details.

https://github.com/user-attachments/assets/22a406ad-0c3c-4fb0-b0a3-0fd65c1a91db

ITEM 2: I can still see the tiles on the SK dashboard even if I turn them off in settings. Is that going to be fixed in a different ticket?

https://github.com/user-attachments/assets/9ed52786-3fc0-4714-baa1-e7ba1ad400e3

____________________________________________

Other than that, the following were tested good.

  • The setup CTA follows the Figma design. ✅
  • The setup CTA has a description with the following copy: To set up new visitor groups for your site, Site Kit needs to update your Google Analytics property. It has an Enable groups link button.✅
  • It should have an
  • Clicking on the "Enable groups" CTA link, the "in progress state" appears according to the ACs and Figma designs. We are then taken to the OAuth flow to grant necessary scopes. ✅
  • Once OAuth granted, the "in progress state" persists until Audience Segmentation is set up, after which, the success notification appears with a switch. Copy and figma are good. ✅
  • Upon success, a dismissible notice is displayed alongside the switch for toggling the audiences widget area.
    • It has the following copy: We've added the audiences section to your dashboard!
    • It has Show me and Got it primary and secondary CTAs.
    • Clicking Show me navigates to the dashboard and scroll sdown to the newly added widget area.
    • Clicking Got it dismisses the notice.
  • Verified on mobile and tablet breakpoints and while there are no figma for those, there is no broken experience.

https://github.com/user-attachments/assets/6a6fc1d7-2f87-4076-a5e6-19d7c7c408db

Screenshot 2024-07-23 at 15 49 06 Screenshot 2024-07-23 at 15 49 16

kelvinballoo avatar Jul 23 '24 12:07 kelvinballoo

@kelvinballoo, regarding your observations:

ITEM 1: First item is regarding the fonts size and weight used. While it's reflecting as per figma, it's not consistent with other text on the page.

We should keep the CTA and text consistent with other admin settings. Please send it back to execution and assign it to @benbowler, as he agreed to work on it during the AS sync.

ITEM 2: I can still see the tiles on the SK dashboard even if I turn them off in settings. Is that going to be fixed in a different ticket?

That has not been implemented yet. @techanvil, could you create a new issue for that?

hussain-t avatar Jul 23 '24 13:07 hussain-t

Thanks @hussain-t! As mentioned on Slack, I've created #9065.

techanvil avatar Jul 23 '24 14:07 techanvil

Back to you for another pass, @kelvinballoo.

techanvil avatar Jul 24 '24 12:07 techanvil

QA Update ✅

Tested ITEM 1 and it's looking much better.

  • The text 'To set up new visitor groups for your site, Site Kit needs to update your Google Analytics property.' is now at 14px, similar to other text on the page.
  • The 'Enable groups' is now at font weight 400
Screenshot 2024-07-24 at 18 21 04

ITEM 2 will be tackled under https://github.com/google/site-kit-wp/issues/9065

Other than that, the following were tested good.

  • The setup CTA follows the Figma design. ✅
  • The setup CTA has a description with the following copy: To set up new visitor groups for your site, Site Kit needs to update your Google Analytics property. It has an Enable groups link button.✅
  • It should have an
  • Clicking on the "Enable groups" CTA link, the "in progress state" appears according to the ACs and Figma designs. We are then taken to the OAuth flow to grant necessary scopes. ✅
  • Once OAuth granted, the "in progress state" persists until Audience Segmentation is set up, after which, the success notification appears with a switch. Copy and figma are good. ✅
  • Upon success, a dismissible notice is displayed alongside the switch for toggling the audiences widget area. ✅
    • It has the following copy: We've added the audiences section to your dashboard!
    • It has Show me and Got it primary and secondary CTAs.
    • Clicking Show me navigates to the dashboard and scroll sdown to the newly added widget area.
    • Clicking Got it dismisses the notice.
  • Verified on mobile and tablet breakpoints and while there are no figma for those, there is no broken experience.
  • Did a smoke test on the main dashboard for enabling groups and it is functioning as expected. Also tested the unhappy paths introduced in #8134. ✅
    https://github.com/user-attachments/assets/eb5a1b09-3fa3-430a-847f-5e4f2d2320b9

https://github.com/user-attachments/assets/6a6fc1d7-2f87-4076-a5e6-19d7c7c408db

Screenshot 2024-07-23 at 15 49 06 Screenshot 2024-07-23 at 15 49 16

Moving ticket to Approval.

kelvinballoo avatar Jul 24 '24 14:07 kelvinballoo