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

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

Open
10 of 12 tasks
jimmymadon opened this issue Apr 29, 2024 · 9 comments
Assignees
Labels
P1 Medium priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@jimmymadon
Copy link
Collaborator

jimmymadon commented Apr 29, 2024

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

  • This branch implements the following points and can be used as a good starting point:
    • 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).
    • 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.
  • 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)

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

  • 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

  • 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

  • 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

  • In addition to verifying the ACs, test the entire Connected Services page thoroughly:
    • Verify that the modules list is now closer in appearance to this new Figma mock. (Perhaps not every aspect is still exact as this is out of the scope of this issue.)
    • Verify that the accordion works, including all the Connected and Not connected / Incomplete Setup buttons.
    • Verify that every settings edit and view for every module still looks fine and is not broken.
    • Verify the AdBlocker warning on both, the Ads and Adsense modules.

Changelog entry

  • Improve Ad Blocker warning messages for Ads and AdSense modules.
@jimmymadon jimmymadon self-assigned this Apr 29, 2024
@jimmymadon jimmymadon added P1 Medium priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature labels Apr 29, 2024
@jimmymadon
Copy link
Collaborator Author

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

@bethanylang
Copy link
Collaborator

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

@jamesozzie
Copy link
Collaborator

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

@bethanylang
Copy link
Collaborator

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.

@jimmymadon
Copy link
Collaborator Author

@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

@sigal-teller
Copy link
Collaborator

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.

@10upsimon
Copy link
Collaborator

AC ✅

@10upsimon 10upsimon removed their assignment May 9, 2024
@jimmymadon jimmymadon assigned jimmymadon and unassigned jimmymadon May 9, 2024
@tofumatt tofumatt self-assigned this May 13, 2024
@tofumatt
Copy link
Collaborator

IB ✅

@mohitwp
Copy link
Collaborator

mohitwp commented May 28, 2024

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 mohitwp assigned jimmymadon and unassigned mohitwp May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants