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 link style within error notices #5975

Closed
aaemnnosttv opened this issue Oct 6, 2022 · 8 comments
Closed

Update link style within error notices #5975

aaemnnosttv opened this issue Oct 6, 2022 · 8 comments
Labels
Good First Issue Good first issue for new engineers P1 Medium priority Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Oct 6, 2022

Feature Description

The current styling of links within error notices looks a bit off and should be updated for better adherence to M3. We should be using an alternate color for on-error surfaces rather than the default link color in this context.

image


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

Acceptance criteria

  • Any link on the red GM2+ background (mainly used for errors) should have the color set to #7A1E00 as per the Figma designs (On Error Container).

Implementation Brief

  • Using assets/sass/components/global/_googlesitekit-cta.scss,
    • Add styles for the .googlesitekit-cta--error a selector whereby the color is #7A1E00 ($c-utility-on-error-container).

Test Coverage

  • No new tests to be added.

QA Brief

  • Doublecheck new CTA Link styling in Storybook
    1. Start storybook
    2. Go to story "Global > Links" and check the External Link
    3. Go to story "Components > Report Error > Default ReportError" and check the external link.

Changelog entry

  • Update link style within error notices.
@aaemnnosttv aaemnnosttv added P1 Medium priority Type: Enhancement Improvement of an existing feature labels Oct 6, 2022
@aaemnnosttv aaemnnosttv self-assigned this Oct 6, 2022
@aaemnnosttv aaemnnosttv removed their assignment Oct 21, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Nov 3, 2022
@eugene-manuilov eugene-manuilov self-assigned this Nov 3, 2022
@eugene-manuilov
Copy link
Collaborator

AC ✔️

@eugene-manuilov eugene-manuilov removed their assignment Nov 3, 2022
@asvinb asvinb self-assigned this Nov 4, 2022
@asvinb
Copy link
Collaborator

asvinb commented Nov 4, 2022

@FlicHollis @eclarke1 Just FYI, even though it's an UI issue, I've estimated it to be 3 since it's a quite straightforward fix and easy to QA.

@asvinb asvinb added the Good First Issue Good first issue for new engineers label Nov 4, 2022
@asvinb asvinb removed their assignment Nov 4, 2022
@techanvil
Copy link
Collaborator

IB ✅

@techanvil techanvil assigned techanvil and unassigned techanvil Nov 11, 2022
@derweili derweili self-assigned this Nov 17, 2022
@derweili
Copy link
Collaborator

@asvinb @techanvil Following the tasks from the IB results in the link-text having the correct #7A1E00 color, but the external-icon still remaining with the wrong #108080 color. As a result, the AC are not met.

This is because the icon is added as a background image, so there is no way for the icon to inherit the text color.

&.googlesitekit-cta-link--arrow,
&.googlesitekit-cta-link--external {
background-image: url("data:image/svg+xml;charset=UTF-8,%3Csvg%20width%3D%2213%22%20height%3D%2213%22%20viewBox%3D%220%200%2013%2013%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%3E%3Cg%20fill%3D%22none%22%20fill-rule%3D%22evenodd%22%3E%3Cpath%20d%3D%22M11%2011H2V2h3V0H2a2%202%200%200%200-2%202v9a2%202%200%200%200%202%202h9c1.1%200%202-.9%202-2V8h-2v3z%22%20fill%3D%22%23108080%22%20fill-rule%3D%22nonzero%22%2F%3E%3Cpath%20fill%3D%22%23108080%22%20d%3D%22M7%200h6v2H7zM11%202h2v4h-2z%22%2F%3E%3Cpath%20d%3D%22M11%202L5%208%22%20stroke%3D%22%23108080%22%20stroke-width%3D%222%22%20stroke-linecap%3D%22square%22%2F%3E%3C%2Fg%3E%3C%2Fsvg%3E");
background-position: calc(100% - 1px) center;
background-repeat: no-repeat;
background-size: 13px 13px;
padding-right: 20px;
}

This means that fixing this bug is not as straightforward as expected in IB.

I assume we need to step back to IB and reconsider how we get the icon to inherit the text color.
This in turn would require a refactoring of the CTA Link CSS (maybe JS, too) that should be handled in a separate issue.

@techanvil
Copy link
Collaborator

techanvil commented Nov 17, 2022

Hi @derweili, thanks for bringing this up. Sometimes details will get missed in an IB and it's usually fine to cover this in the PR, with a note about it under Relevant technical choices which you'll see in the PR template when you create it.

However if you do feel the the change would significantly impact the scope or estimate, then yes it could be appropriate to move the issue back to IB to spec it out or create a new issue as appropriate.

Please take a look, if you think you can fix it in this PR go for it but otherwise feel free to move it back to IB with a note on the issue and we can figure it out from there.

@derweili derweili removed their assignment Nov 18, 2022
@techanvil techanvil assigned techanvil and derweili and unassigned techanvil Nov 21, 2022
@techanvil techanvil removed their assignment Nov 23, 2022
@wpdarren wpdarren self-assigned this Nov 23, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Nov 23, 2022

QA Update: ⚠️

@derweili Hi Julian, I have just one observation. On Storybook: "Global > Links" the External Link isn't the new font color. Should this be the new colour? The components report error is correct.

image

@derweili
Copy link
Collaborator

@wpdarren no, the color should not change on that link page.
Maybe my QA Brief wasn't clear enough.

I meant:

  • Verify the link color on the Global > Links page hasn't changed.
  • Verify the color on the Components > Report Error > Default ReportError page was changed to #7A1E00

Sorry for this confusion.

@wpdarren
Copy link
Collaborator

QA Update: ✅

@derweili thank you for the clarification and no need to be sorry! 👍

Verified:

  • Checked Storybook:

    • Story "Components > Report Error > Default ReportError" has the new color for the external link.
    • Story "Global > Links" for the External Link hasn't changed.
  • Additional QA outside of the QAB. Tested on a site with error messages.

  • Links on the red GM2+ background has the color set to #7A1E00 as per the Figma designs

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants