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

Update module headers in Settings > Connected Services #5620

Closed
techanvil opened this issue Jul 28, 2022 · 29 comments
Closed

Update module headers in Settings > Connected Services #5620

techanvil opened this issue Jul 28, 2022 · 29 comments
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jul 28, 2022

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”.


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/sass/components/settings/_googlesitekit-settings-module.scss, add styles for the .googlesitekit-settings-module__header .mdc-button selector to match the Figma design.

  • 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.
    • Get the store name using the getModuleStoreName selector of the CORE_MODULES store.
    • Get the admin re-auth URL using the getAdminReauthURL selector of the respective module store name.
    • 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 translated string “Complete setup for {module_name}”.
    • Pass the adminReauthURL to the href prop of 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

  • In assets/js/components/settings/SettingsActiveModule/index.test.js, add test coverage for the above scenario.
  • Fix VRT if any fails.

QA Brief

  • Go to Site Kit -> Settings -> Connect More Services.
  • Start connecting a service but do not complete, abandon the process in the middle.
  • Go to Site Kit -> Settings -> Connected Services.
  • Verify that the connected services only display a Connected string instead of {module} is connected.
  • Verify 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 should take you to the module setup flow.
  • Verify that clicking on the button doesn't open the respective panel (as reported here).

Changelog entry

  • On the modules headers in the Settings > Connected Services tab, update the status text and show a CTA for continuing the module setup.
@techanvil techanvil added P1 Medium priority Type: Enhancement Improvement of an existing feature labels Jul 28, 2022
@techanvil techanvil changed the title Add buttons in Settings > Connected Services to open settings in edit mode Update module headers in Settings > Connected Services Aug 3, 2022
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Aug 9, 2022
@tofumatt
Copy link
Collaborator

tofumatt commented Aug 9, 2022

ACs look good; moving this to IB. 👍🏻

@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Aug 10, 2022
@eugene-manuilov eugene-manuilov self-assigned this Aug 10, 2022
@eugene-manuilov
Copy link
Collaborator

  • 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

@techanvil
Copy link
Collaborator Author

@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 techanvil assigned hussain-t and unassigned techanvil Aug 10, 2022
@eugene-manuilov
Copy link
Collaborator

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.

@aaemnnosttv
Copy link
Collaborator

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.

@eugene-manuilov
Copy link
Collaborator

@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.

@aaemnnosttv
Copy link
Collaborator

@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.

@eugene-manuilov
Copy link
Collaborator

@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.

@aaemnnosttv
Copy link
Collaborator

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.

@hussain-t
Copy link
Collaborator

hussain-t commented Aug 29, 2022

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.

@eugene-manuilov
Copy link
Collaborator

eugene-manuilov commented Aug 29, 2022

  • 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

<Link
className={ classnames( 'googlesitekit-settings-module__header', {
'googlesitekit-settings-module__header--open': isOpen,
} ) }
id={ `googlesitekit-settings-module__header--${ slug }` }
type="button"
role="tab"
aria-selected={ isOpen }
aria-expanded={ isOpen }
aria-controls={ `googlesitekit-settings-module__content--${ slug }` }
to={ `/connected-services${ isOpen ? '' : `/${ slug }` }` }
onClick={ onHeaderClick }
>

techanvil added a commit that referenced this issue Sep 2, 2022
…-headers

Updated connected module settings headers
@techanvil techanvil removed their assignment Sep 2, 2022
@mohitwp mohitwp self-assigned this Sep 2, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Sep 5, 2022

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.

@wpdarren
Copy link
Collaborator

wpdarren commented Sep 5, 2022

@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.

@mohitwp
Copy link
Collaborator

mohitwp commented Sep 5, 2022

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

@wpdarren
Copy link
Collaborator

wpdarren commented Sep 5, 2022

@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 wpdarren assigned nfmohit and unassigned wpdarren Sep 5, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Sep 5, 2022

@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 nfmohit removed their assignment Sep 5, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Sep 5, 2022

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 mohitwp removed their assignment Sep 5, 2022
@felixarntz
Copy link
Member

felixarntz commented Sep 8, 2022

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.

@nfmohit
Copy link
Collaborator

nfmohit commented Sep 8, 2022

@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.

@felixarntz
Copy link
Member

@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!

@nfmohit nfmohit self-assigned this Sep 9, 2022
@nfmohit nfmohit removed their assignment Sep 9, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Sep 9, 2022

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

@eugene-manuilov eugene-manuilov self-assigned this Sep 9, 2022
eugene-manuilov added a commit that referenced this issue Sep 9, 2022
…open

Do not open accordion on action click
@eugene-manuilov eugene-manuilov removed their assignment Sep 9, 2022
@wpdarren wpdarren self-assigned this Sep 9, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Sep 9, 2022

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.
settings.mp4

@wpdarren wpdarren removed their assignment Sep 9, 2022
@aaemnnosttv aaemnnosttv self-assigned this Sep 9, 2022
@aaemnnosttv aaemnnosttv removed their assignment Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants