Buttons with "secondary content" colour should be standardized

[mxbclang]

Bug Description

The colour of the "Change Metrics" CTA button differs from every other link button in the plugin. This override also causes the hover colour to be wrong and the icon's colour not to match.

As reported by @sigal-teller in Bug Bash.

---

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

Acceptance criteria

• The colour of the "Change Metrics" CTA button should use $c-content-secondary, but this should not cause issues with its hover styling. Its icon should also match the colour of text used for the button.
• There should be a prop-based way to enable the "secondary content colour" (eg. $c-content-secondary) colour for button links, including their icons and appropriate hover colours.
• Existing instances (outlined in this comment below: https://github.com/google/site-kit-wp/issues/7479#issuecomment-1699003924) should be refactored to use a prop for the Button instead of the CSS override.

Implementation Brief

• To address AC, we can treat current global CTA style (.googlesitekit-cta-link in assets/sass/components/global/_googlesitekit-cta-link.scss) as primary. Only missing piece to add there is default color for svg icons when present. So add there (approximately after this line https://github.com/google/site-kit-wp/blob/3363c620630c13317d2dace0e5f0ff892f804041/assets/sass/components/global/_googlesitekit-cta-link.scss#L28C1-L28C1 ):

  svg {
  fill: $c-link;
  }

• Then include a new button class there that will globally apply secondary styling with $c-content-secondary. So in that same file, add new class slector at the same level as &.googlesitekit-cta-link--caps :

  &.googlesitekit-cta-link--secondary {
  color: $c-content-secondary;
  
  svg {
      fill: $c-content-secondary;
  }
  
  &:hover {
      color: $c-content-secondary;
  
      svg {
          fill: $c-content-secondary;
      }
  }
  }

• Then adapt assets/js/components/Link.js component to accept new prop secondary. And scroll down to where LinkComponent component is returned and pass new prop pair to the object (third argument of classnames function), like this: 'googlesitekit-cta-link--secondary': secondary

Apply new secondary prop

Include new prop secondary to the Link component in following files:

• assets/js/components/KeyMetrics/ChangeMetricsLink.js then delete color override Here

• assets/js/components/EntityHeader.js, and then remove color override from here

• assets/js/components/KeyMetrics/MetricsSelectionPanel/Footer.js in this Link and then remove color override Here

• assets/js/components/user-input/UserInputPreviewGroup.js in Link component under googlesitekit-user-input__preview-group-title div. And then remove color override here

• Check if this https://github.com/google/site-kit-wp/blob/3363c620630c13317d2dace0e5f0ff892f804041/assets/sass/components/settings/_googlesitekit-settings-notice.scss#L175 line is actually appied to something. So far the only place using this notice class is this component: assets/js/components/SettingsNotice/SettingsNotice.js, which does not include Link component anywhere, only Button.

Test Coverage

• Update VRTs/snapshot tests as-needed.
• Add a new story/Visual Regression Test for this new secondary-styled Link component, including one with an icon.

QA Brief

• The icon colour and text colour of the "Change Metrics" CTA (see screenshot above) should match.

Changelog entry

• Ensure icon and link/button colors are shared.

[tofumatt]

@zutigrm The ACs here mention that if this is considered an issue for this button, we should change all styles. Looks like this hover effect was intentionally done on the link buttons (as part of the GM2+ styles).

We should remove the hover colour change everywhere though; there's no sense in making this link button a special case. Looks like this IB would only change that particular instance, right?

[zutigrm]

@tofumatt Yes, it will only change local instance. Since the global style is fine, as referenced in first file, it sets primary color for text and then hover is keeping the same color which ensures that color is unchanged. So global styles need no change.

Only the referenced button needs change per IB, since it is that button specific and it is using secondary color, overriding the global, but since it does not have a hover style, color on hover is inherited from global button primary color, causing the color change. So we only need to implement the hover as suggested for local button.

Removing the hover globally will affect all buttons, which means their hover style can be inherited from difference selectors.

[tofumatt]

Ohhhhh gotcha. Okay, neato.

I wonder why we bother to override that one specific button's style in the first place? 🤔

This is the current colour scheme, which ends up needing this awkward hover override and has a mismatched icon colour:

This is what we get if we remove the override:

Let's just do that instead, because a one-off colour replacement and then override is quite odd. If we should change this colour, let's do it globally.

I've amended the ACs and IB here, because I don't think we should be changing this in the first place. It's likely from a change to colours in a Figma mock, but we shouldn't be one-offing CTA buttons.

[tofumatt]

Oh, actually, we do this elsewhere too and seemingly have weird overrides for it: https://github.com/google/site-kit-wp/blob/3363c620630c13317d2dace0e5f0ff892f804041/assets/sass/components/global/_googlesitekit-entity-header.scss#L123-L131

Seems like we override the button link colours in these places:

• https://github.com/google/site-kit-wp/blob/3363c620630c13317d2dace0e5f0ff892f804041/assets/sass/components/global/_googlesitekit-entity-header.scss#L123-L131
• https://github.com/google/site-kit-wp/blob/3363c620630c13317d2dace0e5f0ff892f804041/assets/sass/components/key-metrics/_googlesitekit-km-selection-panel.scss#L129
• https://github.com/google/site-kit-wp/blob/3363c620630c13317d2dace0e5f0ff892f804041/assets/sass/components/settings/_googlesitekit-settings-notice.scss#L175
• https://github.com/google/site-kit-wp/blob/3363c620630c13317d2dace0e5f0ff892f804041/assets/sass/components/user-input/googlesitekit-user-input-preview.scss#L137

This issue isn't specific to Key Metrics, it's just that it was noticed here. We should fix this in the actual button component by supplying proper color, :hover, etc. values and also making sure attached icons use the right colour.

I'm removing this from the KMW epic and will adjust the ACs.

[tofumatt]

I've adjusted the ACs here and removed the existing IB, because this will actually involve changing all of the buttons mentioned above: https://github.com/google/site-kit-wp/issues/7479#issuecomment-1699003924.

Should be good for a new IB now, but it's probably a bit more involved than the original issue 🙂

[zutigrm]

@tofumatt I updated IB to include primary and secondary color and prop globally in Link component. The only thing not included is special case of usage of external icons.

For some reason they are included as inline svg, and not present as physical file in svg folder. The IB address icon color, when icon is added regular way via Icon component. Due to the nature of how links with external class and icon are added, not sure if we should address the icon change in this issue.

It can have certain impact - switching from it being rendered as bg image through css to locating proper svg and including it via icon component, and then potential sizes that might be affected/ of imported svg vs inline data, as well as locating all instances where this link with external prop is located and including icon + secondary prop on link, etc. I think we should do this in separate issue, what is your opinion on that?

[tofumatt]

@zutigrm In this case I think the icon colours being off is related to the colours not being standardised/applied consistently.

But you're right, there are a lot of link icons in CSS that we can't style, which isn't ideal: https://github.com/google/site-kit-wp/blob/e3fa57495464031113a62449db5f5e0afdac8e6c/assets/sass/components/global/_googlesitekit-cta-link.scss#L52-L143

From what I can see, we can change these to use inline icons instead but I think it should be a related issue. But I'd argue it blocks this one, as we should then be able to control the colour of these icons with fill or similar.

I've filed #7542 as a blocker to this issue. The IB here can be completed assuming all Link icons with SVGs in the CSS will be converted to SVGs in the markup that can be styled.

[zutigrm]

@tofumatt Thanks. Yes, I thought it would be better as separate issue due to easier testing, and making it into 2 smaller tasks vs lots of changes in this one. Doesn't need to be blocked by external icons change, as you pointed out, since this issue will resolve main issue - color overrides, and when external icons are moved as icons component this change will apply to them as well.

[tofumatt]

@zutigrm Looks good, but I've just added a note that we should have storybook stories/VRTs for this new link variant, including icons. That way we can be sure it keeps working in the future 🙂 . Because of that I've upped the estimate a bit 🙂

IB ✅

[mohitwp]

QA Update ✅

• Tested on dev environment.
• Verified 'Change Metrics' CTA icon and text color are now matching.
• Verified on hover 'Change Metrics' color is now #108080

**Latest** -  **Develop -**