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

Update module headers in Settings > Connected Services

Open techanvil opened this issue 3 years ago • 9 comments

Feature Description

For unconnected modules on the Settings > Connected Services tab, display a CTA button in place of the existing “{module_name} is not connected” text. The button text should read “Complete setup for {module_name}”. Clicking on the button should redirect to the Setup page for the module.

Additionally, for a connected modules the text should be changed from the existing “{module_name} is connected” to simply “Connected”.

  • Design Doc: https://docs.google.com/document/d/1DeWo38lcV7lvLFfcmt-mQ0iaxzB4qfiPArs_yZzYkIM/edit?pli=1#heading=h.2v5n801ctnc3
  • Figma: https://www.figma.com/file/vMaCWwr6lpk4PrJWb7jIpz/GA4-Banner-Input?node-id=1285%3A2032

Acceptance criteria

  • On the Settings > Connected Services tab, for unconnected modules the existing “{module_name} is not connected” text should no longer appear.
  • In its place a grey CTA button should displayed with text “Complete setup for {module_name}”.
  • The new triangular warning icon should be displayed in yellow as per Figma.
  • Clicking on the button should navigate to the Setup page for the module.
  • For connected modules, the existing “{module_name} is connected” text should be replaced with the single word “Connected”.

Implementation Brief

  • In assets/js/components/Button.js, add the following code:

    • Add a new boolean prop called secondary to the Button component.
    • Add a new className mdc-button--secondary and set the value secondary obtained from the props.
  • In assets/sass/vendor/_mdc-button.scss, add the styles for the new &--secondary class with the styles that should match the Figma design.

  • In assets/js/components/settings/SettingsActiveModule/Header.js, modify the following:

    • Render the existing <p> tag only if the module is connected.
    • Replace the existing “{module_name} is connected” text with the single word “Connected”.
    • Remove the else part from the ternary operator.
    • Render the Button component only if the module is not ! connected with the text “Complete setup for {module_name}”.
    • Pass secondary as a prop to the Button component.
    • Render the span tag (icon) as a sibling of the Button component.
  • In assets/sass/components/settings/_googlesitekit-settings-module.scss, modify the icon class googlesitekit-settings-module__status--not-connected to match the Figma designs.

Test Coverage

  • Fix VRT if any fails.

QA Brief

Changelog entry

techanvil avatar Jul 28 '22 13:07 techanvil

ACs look good; moving this to IB. 👍🏻

tofumatt avatar Aug 09 '22 17:08 tofumatt

  • Clicking on the button should expand the module settings in edit mode.

@techanvil, modules like AdSense and TwG show different setup flows based on some parameters, which will be impossible to reproduce if we show to users just the settings edit form. How is it supposed to work? Perhaps we should just redirect users to the setup page if they click on that button?

cc @felixarntz @aaemnnosttv

eugene-manuilov avatar Aug 10 '22 12:08 eugene-manuilov

@eugene-manuilov, thanks for raising this - it's a really good point.

I had forgotten that the edit versions of the expanded sections for modules which are not yet setup are functionally limited and just include a link to the setup page rather than an embedded setup UI. In other words they tend to look like this:

image

This is in contrast to the requirement for the Analytics page when it is setup, but GA4 is not yet connected - in this case expanding the module in edit mode is OK because the edit UI is displayed inline.

So, I think you are right - we should redirect users to the corresponding setup page when they click on the "Complete setup for {module_name}" CTA.

@hussain-t, I have updated the AC to specify navigating to the setup page, please could you update the IB accordingly?

cc @felixarntz @aaemnnosttv

techanvil avatar Aug 10 '22 16:08 techanvil

I have updated the AC to specify navigating to the setup page,

Thanks, @techanvil, but after reviewing IB for #5621 it turns out that things are more complicated than I thought at the beginning... Redirecting users to the setup page is the default behaviour for that button, but we need to allow modules to override it because for the Analytics module we need a special behaviour that will open the settings edit form and point the user to the GA4 property dropdown.

That's said, we need to allow modules define a custom CTA button similarly how we do it for the SettingsSetupIncompleteComponent element. If a module needs its own behaviour for the CTA button, it will need to define a component that will be responsible for rendering the button and handling clicks on it. If a module doesn't define such component, then the default one will be used that will redirect users to the setup page. What do you think? I'll move this ticket to AC and will assign it to you for now.

eugene-manuilov avatar Aug 12 '22 12:08 eugene-manuilov

we need to allow modules to override it because for the Analytics module we need a special behaviour that will open the settings edit form and point the user to the GA4 property dropdown.

@eugene-manuilov I'm not sure we need to allow modules to override this. I feel like we could update the setup UI to highlight the GA4 field if GA4 isn't connected yet without requiring more substantial changes here. In this case, we could use the same criteria to determine if that should be done (UA connected, and GA4 not). This could be a separate issue to implement where we further define what that should look like and the form it takes. Thoughts?

Otherwise, I think your suggested approach sounds reasonable, I'm just not sure it's necessary.

aaemnnosttv avatar Aug 12 '22 17:08 aaemnnosttv

@aaemnnosttv I think in this case it's necessary if we want to keep our settings API consistent. We had an edge case for AdSense and we added the ability to define the SettingsSetupIncompleteComponent property/component on per-module basis, now we have an edge case for GA4 set up and we need to use the similar approach to resolve it.

eugene-manuilov avatar Aug 12 '22 17:08 eugene-manuilov

@eugene-manuilov I think there is some confusion about what the intended outcome is due to some of the changes that have happened in the definition here. This issue was originally about adding the button to open the module settings in edit mode but we have since refined that to point to the module setup as a more appropriate destination.

The ACs for #5621 however still reference opening the module settings in edit mode, which I think should be updated to reference the setup screen instead. IMO this is still relevant here and solves the problem of "enabling" GA4 on the settings edit screen as GA4 is always required as part of Analytics setup now and we're essentially completing the setup for GA4. The only addition we'd need is the added tooltip highlighting the GA4 property field which could be done conditionally in the existing setup component as long as UA is already connected.

Defining a new component to customize the [finish setup CTA] this issue adds would still require some special case for GA4 somewhere because the SettingsActiveModule in settings is for analytics, not analytics-4.

aaemnnosttv avatar Aug 12 '22 17:08 aaemnnosttv

@aaemnnosttv ok, i am fine with this if we update ACs for #5621. Do we need to update design mocks in Figma as well? Darren will compare the behaviour in the plugin with what we have in Figma and will raise an error.

Will move this ticket back into IB and assign to Hussain.

eugene-manuilov avatar Aug 12 '22 19:08 eugene-manuilov

Do we need to update design mocks in Figma as well? Darren will compare the behaviour in the plugin with what we have in Figma and will raise an error.

Yes, good call. Let's first confirm this change with UX, and then we can have the design updated to reflect this. The added callout may not even be necessary if setting up GA4 is a requirement for completing setup as it should be.

aaemnnosttv avatar Aug 12 '22 20:08 aaemnnosttv

Thanks, @eugene-manuilov, @aaemnnosttv, and @techanvil. I have updated the IB accordingly. I have increased the estimate by considering adding tests.

I have added a point for calling the trackEvent when the Complete setup for {module_name} button is clicked. LMK if that sounds good.

hussain-t avatar Aug 29 '22 10:08 hussain-t

  • In assets/js/components/Button.js, add the following code:

    • Add a new boolean prop called secondary to the Button component.
    • Add a new className mdc-button--secondary and set the value secondary obtained from the props.
  • In assets/sass/vendor/_mdc-button.scss, add the styles for the new &--secondary class with the styles that should match the Figma design.

@hussain-t, I think we can do the same without introducing a new property in the Button component. We can just add styles for the .googlesitekit-settings-module__header .mdc-button selector to make the button look as we need.

  • In assets/js/components/settings/SettingsActiveModule/Header.js, modify the following:

I think if we implement these changes, we will get a button in the a tag which is wrong. I think we need to rework the Header component to use the <div> tag with the onClick handler instead of Link. cc @tofumatt

https://github.com/google/site-kit-wp/blob/96e62dfed0f192e982d796c18fe27152a98e51b7/assets/js/components/settings/SettingsActiveModule/Header.js#L78-L90

eugene-manuilov avatar Aug 29 '22 16:08 eugene-manuilov

Thanks for pointing it out, @eugene-manuilov. I have updated the IB as per the change request.

hussain-t avatar Aug 31 '22 09:08 hussain-t

Thanks, @hussain-t. IB ✔️

eugene-manuilov avatar Aug 31 '22 09:08 eugene-manuilov

  • In assets/js/components/settings/SettingsActiveModule/Header.js, modify the following:
    • The main Link component should be replaced with a div element.
    • Fix the lint and accessibility errors for the above change. For example, tabIndex and onKeyDown properties need to be added.
    • Remove the to prop and update the onHeaderClick callback with the following:
      • Using the useHistroy hook, update the URL with the value of to prop when the module is connected.
      • Update the logic to call the trackEvent only if the module is connected.
    • Potentially, we could have a separate or a modified (the same) callback for “Complete setup for {module_name}” and pass it to the onClick prop of the Button component.

@hussain-t Just to confirm, do we want the modules that are not connected to not open, i.e. should clicking on the .googlesitekit-settings-module__header element for ! connected modules not do anything? If that is the case, should we remove the cursor: pointer and the arrow at the very right for ! connected modules?

CC: @eugene-manuilov

nfmohit avatar Aug 31 '22 22:08 nfmohit

@nfmohit, when double-checking the Figma design and the comments related to it, it seems we should not open the accordion. The gray CTA button indicates it's disabled. However, there are a couple of concerns:

  1. Should we change the color of the chevron to gray, i.e., to indicate it's disabled?
Screenshot 2022-09-01 at 11 06 13 AM
  1. One other thing is that the expanded accordion has the edit->disconnect module option. Otherwise, there is no way to disconnect a module when the setup is incomplete.
Screenshot 2022-09-01 at 11 40 46 AM

@techanvil, could you shed some light on this?

cc: @aaemnnosttv

hussain-t avatar Sep 01 '22 06:09 hussain-t

@nfmohit, when double-checking the Figma design and the comments related to it, it seems we should not open the accordion. The gray CTA button indicates it's disabled. However, there are a couple of concerns:

  1. Should we change the color of the chevron to gray, i.e., to indicate it's disabled?
Screenshot 2022-09-01 at 11 06 13 AM
  1. One other thing is that the expanded accordion has the edit->disconnect module option. Otherwise, there is no way to disconnect a module when the setup is incomplete.
Screenshot 2022-09-01 at 11 40 46 AM

@techanvil, could you shed some light on this?

cc: @aaemnnosttv

Hi @hussain-t, actually we should still be opening the accordion. The concern is that the grey CTA looks like it's disabled, but it's not, and UX will work on the design aspect to better indicate the fact that it is indeed active.

So, we don't need to do anything with the chevron, and will still be able to expand the accordion and thus disconnect a module.

cc @nfmohit

techanvil avatar Sep 01 '22 08:09 techanvil

Thanks, @techanvil. That brings another question, what do we do with the Setup incomplete: [continue module setup](http://sitekit.10uplabs.com/wp-admin/admin.php?page=googlesitekit-dashboard&slug=adsense&reAuth=true) link?

Screenshot 2022-09-01 at 11 21 16 AM

hussain-t avatar Sep 01 '22 08:09 hussain-t

That brings another question, what do we do with the Setup incomplete: continue module setup(http://sitekit.10uplabs.com/wp-admin/admin.php?page=googlesitekit-dashboard&slug=adsense&reAuth=true) link?

@hussain-t we should do nothing with that link, it should remain as is.

Just to confirm, do we want the modules that are not connected to not open, i.e. should clicking on the .googlesitekit-settings-module__header element for ! connected modules not do anything? If that is the case, should we remove the cursor: pointer and the arrow at the very right for ! connected modules?

@nfmohit, no, modules that are not connected yet should still open when you click on the header itself, that's why we change the <Link> component with the <div onClick={...}> tag.

eugene-manuilov avatar Sep 01 '22 09:09 eugene-manuilov

QA Update

@nfmohit

Verified this ticket for other modules except GA4 because GA4 notifications are currently not appearing on develop branch. I will verify this for GA4 module when issue will get resolve.

image

Assigning to @wpdarren if he can verify this for GA4 module using elasticpress.io site.

mohitwp avatar Sep 05 '22 01:09 mohitwp

@mohitwp Just left a message with you on Slack but I am unsure what this has to do with GA4 notification banner? I can't see any reference to it in the QAB. This ticket is to check the incomplete set up of the modules.

wpdarren avatar Sep 05 '22 02:09 wpdarren

QA Update ⚠️

@nfmohit In Figma design for if Analytics is not connected-text is "Complete setup for Google Analytics 4". Ideally, it should be "Complete setup for Analytics" and currently it is. But just wants to confirm if we are showing this text ""Complete setup for Google Analytics 4" in case of any specific scenario?

Figma image

cc @wpdarren

mohitwp avatar Sep 05 '22 02:09 mohitwp

@nfmohit @mohitwp thanks for the update here 👍 I have just checked this and I think the figma design for this is probably future proofed for when we move to fully-implementing GA4. This is what I see:

image

Would be good for Nahid to confirm this though.

wpdarren avatar Sep 05 '22 03:09 wpdarren

@wpdarren @mohitwp Thank you for checking.

The behaviour that you're seeing is correct. The Figma design that @mohitwp referenced to actually gets accomplished in #5621. "Connect Google Analytics 4" should only be shown when Analytics is setup but Analytics 4 is not, once #5621 is executed.

nfmohit avatar Sep 05 '22 03:09 nfmohit

QA Update ✅

Verified

  • Verified that the connected services only display a Connected string instead of {module} is connected.
  • Verified that the service which was abandoned in the setup process shows a button that says Complete setup for {module} with a slightly changed icon according to Figma, clicking on which takes user to the module setup flow.
  • Compared from figma design.
  • Verified in mobile devices.

image

mohitwp avatar Sep 05 '22 12:09 mohitwp

Approval ⚠️

@hussain-t @eugene-manuilov @techanvil It's odd UX here that when clicking one of the "Complete setup for ..." buttons the panel still opens before you get taken to the setup screen. We should make sure the button click in that case does not toggle the panel. All the rest of the panel header should still toggle it, just not the button within it.

felixarntz avatar Sep 08 '22 17:09 felixarntz

@felixarntz I noticed it post merge (should've noticed earlier, my bad, apologies) and included a fix for it in #5803 (#5621). I can create a new PR addressing only this in the meantime if preferred.

nfmohit avatar Sep 08 '22 17:09 nfmohit

@nfmohit A separate PR just to fix this for here would probably be best, since #5621 may only land in the following release, while this here will go out to users next week. Thank you!

felixarntz avatar Sep 08 '22 18:09 felixarntz

@felixarntz Understood. Added a new PR targeting main. Thank you!

nfmohit avatar Sep 09 '22 09:09 nfmohit

QA Update: ✅

  • The connected services only display a Connected string instead of {module} is connected.
  • When the service was abandoned in the setup process it shows a button that says Complete setup for {module} with a slightly changed icon. Clicking on it takes you to the module setup flow.
  • When clicking on the button it doesn't open the respective panel.

https://user-images.githubusercontent.com/73545194/189362615-bd39763d-bb88-4a30-a22f-83c367acf736.mp4

wpdarren avatar Sep 09 '22 13:09 wpdarren