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

Standardise "Incomplete Setup" list in Settings for Ads and AdSense modules when AdBlocker is detected

Open jimmymadon opened this issue 1 year ago • 8 comments

Feature Description

  1. As per point 2 of this comment, when an Ad Blocker is detected, a "Learn more" link should be displayed similar to the equivalent component for AdSense.

  2. Furthermore, these two components have different font sizes and should be standardised. The Ads module uses the DefaultSettingsSetupIncomplete component which uses the generic ModuleSettingsWarning component. This has a smaller font for any requirementsError messages. The AdSense module uses a custom SettingsSetupIncomplete component which then wraps the AdBlockerWarning component that contains the Get Help link as well.

Ads Incomplete Setup AdBlocker requirements error message: Screenshot 2024-04-17 at 15 13 15

AdSense Incomplete Setup AdBlocker Warning: Screenshot 2024-04-17 at 15 13 26

  1. Finally, as per this comment, there is a header separator for the Ads and AdSense modules' settings list that make the settings accordion look "better" when the AdBlocker Warning message is shown. This separator does not exist on other modules.

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

Acceptance criteria

  • In the "Connected Services" modules list, the following changes should be made as per this Figma mock: ConnectedServices
  • The colour of the "Complete setup for X" should be updated (black instead of grey).
  • Within the Ads and AdSense module sections, the horizontal rule (line) should be removed during setup (i.e. when connecting the modules).
  • The horizontal rule (divider) should also be removed for all modules in the modules list when opening the accordion and viewing/editing settings (Figma).
  • Within the Ads and AdSense module sections, the Ad Blocker detected warning should be redesigned as per our new warning message pattern. All other generic warning / error messages other than Ad Blocker detection should also follow the same design.
  • The Get help link should be added to the Ads section's Ad Blocker warning message, the same way we do for the AdSense module.
  • When the AdBlocker error warning is rendered, the "Complete setup for X" button and "continue module setup" link should be in the disabled state (grey instead of black / green).
  • The Ads, AdSense, Tag Manager and Analytics Setup screens should all have standardized logo, heading and spacing as per the new Ads Figma mock.

Implementation Brief

  • [x] This branch implements the following points and can be used as a good starting point:
    • [x] assets/sass/components/settings/_googlesitekit-settings-module.scss, update the colour of the "Complete setup of module x" button (mdc-button within googlesitekit-settings-module__header).
    • [x] In assets/js/modules/ads/components/setup/SetupMainPAX.js, assets/js/modules/ads/components/setup/SetupMain.js, remove the div with class googlesitekit-setup-module__step. This removes the header divider.

Removing the divider

  • [ ] In assets/js/modules/adsense/components/setup/SetupMain.js, remove the googlesitekit-setup-module__step as above, however, add necessary margins or padding for the content based on different setup components that the AdSense setup form can contain.
  • [x] In assets/sass/components/settings/_googlesitekit-settings-module.scss, for the googlesitekit-settings-module__content--open class, add border: none; to remove the divider when the accordion is opened.

Restyling the Ad Blocker warning message (and other warnings in the list)

  • [x] In assets/js/components, create a new reusable component, WarningNotice, which is similar in desing to the SettingsNotice component with type=warning. However, the colour of the font should be $c-utility-on-warning-container. This component also will not have any icon. Ensure that any links within this component are in bold and have the same colour as the text.
  • [x] In assets/js/components/AdBlockerWarningMessage.js, use the above new WarningMessage component to render the message. Remove the additional ErrorIcon. Ensure that this warning is displayed correctly on the dashboard, in the "Connect more services" modules list, in the "Incomplete setup" modules list accordion as well as during module setup.
  • [x] In assets/js/components/notifications/ModuleSettingsWarning.js, use the above WarningMessage component and remove the ErrorIcon. Remove any unused styles related to this component and ensure it is rendered correctly within the "Connect more services" modules list as well as the "Incomplete setup" list in "Connected services".

Adding Get help link to the Ads module

  • [x] Similar to AdSense module, create a SettingsSetupIncomplete component in assets/js/modules/ads/components/settings/, which simply renders the AdBlockerWarning component.

Disabling the "Complete module setup" link

  • [x] In both the SettingsSetupIncomplete components for Ads and AdSense, use the getCheckRequirementsError selector again to check if there is an error. If there is, then pass the disabled prop to the Link component for the "Complete module setup" link.

Disabling the "Complete setup for " button

  • [x] In assets/js/components/settings/SettingsActiveModule/Header.js, use the getCheckRequirementsError selector from the core/modules store. If there is an error, then in the moduleStatus button, pass the disabled prop to be true.
  • [ ] In assets/sass/components/settings/_googlesitekit-settings-module.scss, style this button as per the figma mock for the disabled state.

Test Coverage

  • No new tests required.

QA Brief

Changelog entry

jimmymadon avatar Apr 29 '24 07:04 jimmymadon

@sigal-teller Based on the feature description of this issue, could you please advise on:

  1. Point 2: Which font / design to use for the AdBlocker Warning component between the two existing screenshots.
  2. Point 3: Should we keep the "header separator" for the modules settings view in Ads / AdSense and add it to the other modules? Or should we remove it from everywhere?

jimmymadon avatar Apr 29 '24 10:04 jimmymadon

@jimmymadon The support team will need to work on creating a guide for the Learn more link and getting it uploaded to the service. @adamdunnage @jamesozzie Can you please coordinate with Jimmy here? Thanks!

mxbclang avatar Apr 29 '24 11:04 mxbclang

@jimmymadon @bethanylang The Get Help link already exists (with an ads-ad-blocker-detected error code). This is present in the Support Redirects sheet.

The link as per the sheet above directs users to https://sitekit.withgoogle.com/documentation/troubleshooting/ads/#ad-blocker-detected, as expected. I don't see the "Get Help" link at present when testing on a site, instead I see the below greyed out module setup screen when an Ad Blocker is detected. image

Happy to test once this is ready, or to answer any questions on the comments above.

jamesozzie avatar Apr 29 '24 11:04 jamesozzie

Thanks @jamesozzie! Yes, this is just in Acceptance Criteria now, so we haven't done anything on it just yet. :) Just wanted to make sure it was on the support team's radar.

mxbclang avatar Apr 29 '24 11:04 mxbclang

@bethanylang Actually, the 'Learn more' link is missing only on the component mentioned in the screenshots of this issue's feature description. It is already present in other places where it should be, viz. on the setup and settings forms where the AdBlocker warning is displayed. As @jamesozzie has mentioned, it is live and working from these places.

E.g. of the Setup form: Screenshot 2024-04-29 at 18 10 05

jimmymadon avatar Apr 29 '24 12:04 jimmymadon

Hey @jimmymadon I reviewed this issue. Since this was never actually properly designed but was assembled from various components the result can be improved. I’m listing the changes that I think we need to implement, with a link to the relevant Figma design I created for it.

  1. The complete setup for AdSense button shouldn’t be gray. We don’t use gray buttons anywhere else, only primary green or black. The gray one looks like a disabled state. I already updated it while working on Ads module, you can find it here.

  2. We shouldn’t have a divider below the title, because it’s the same one that is being used as a divider between the modules. It makes it harder to understand that the settings below are a part of the module and relates to the title above. You can see that I didn’t add a divider in my designs too wherever I designed something for settings.
  3. As for the warning text, I think that this is not a good UI, and maybe we can use here the same pattern that we’re using in other warning notifications to create consistency. We also don’t need to user the warning icon again if it shows at the top of the module to the right. I created a Figma design for it here I know that this is an additional effort but it will unify these type of warning notifications.

sigal-teller avatar Apr 30 '24 10:04 sigal-teller

AC ✅

10upsimon avatar May 09 '24 13:05 10upsimon

IB ✅

tofumatt avatar May 13 '24 21:05 tofumatt

QA Update ❌

  • Tested on dev environment.
  • Verified all the points mentioned under AC and QAB.
  • Also, compared new design with the Figma.

@jimmymadon Please find my observations below :

1) 'Complete setup for X' button text color in disable state is different from figma.

Figma

image

image

2) 'Complete setup for X' button background color in disable state is different from figma.

image

Figma

image

3) 'Complete setup for X' button text color in enabled state is different from figma.

image

Figma

image

4) 'Setup Complete' text Font weight is not same as Figma.

image

Figma

image

5) 'Get help" SVG icon looks little misalign and have different size than the figma.

image

image

Figma

image

6) Icons, button and text alignment between accordions is not same as Figma.

Actual Implementation

image

Figma

image

Question >When both the Ads and AdSense modules are connected and an AdBlocker is detected, we show a notice when the user tries to access the Ads module settings. However, the same notice does not appear when the user tries to access the AdSense module settings. This behavior is consistent with the latest environment. However, when we show a notice on the main dashboard for AdSense, I think we should display the same notice under settings as well. Let me know your thoughts.

image

image

mohitwp avatar May 28 '24 21:05 mohitwp

@mohitwp

Question When both the Ads and AdSense modules are connected and an AdBlocker is detected, we show a notice when the user tries to access the Ads module settings. However, the same notice does not appear when the user tries to access the AdSense module settings.

In #8515, we stopped showing settings for Ads as that needed a request that was being blocked by AdBlockers and the settings view and edit pages would throw errors. However, the same does not happen on the AdSense settings pages for now - so we continue to show these without any warning. This is a conscious decision so I suppose this is fine as is for now.

All other points have been addressed in the attached follow up PR.

c.c. @tofumatt

jimmymadon avatar May 29 '24 23:05 jimmymadon

QA Update ✅

  • Tested on main environment.
  • Verified all the points mentioned under AC and QAB.
  • Also, compared new design with the Figma.
  • Verified all issues reported above are now resolved

1) 'Complete setup for X' button text color in disable state is different from figma. (PASS)

image

2) 'Complete setup for X' button background color in disable state is different from figma. (PASS)

image

3) 'Complete setup for X' button text color in enabled state is different from figma. (PASS)

image

4) 'Setup Complete' text Font weight is not same as Figma. (PASS)

image

5) 'Get help" SVG icon looks little misalign and have different size than the figma. (PASS)

image

6) Icons, button and text alignment between accordions is not same as Figma. (PASS)

image

mohitwp avatar May 30 '24 06:05 mohitwp

Nice improvement 👍

aaemnnosttv avatar May 31 '24 20:05 aaemnnosttv