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

chore(links): update visited links colors and application #1735

Merged
merged 12 commits into from
May 20, 2024

Conversation

dancormier
Copy link
Contributor

@dancormier dancormier commented May 14, 2024

STACK-481

This PR updates the visited link style to purple-500 (and purple-600 when visited and hovering) where appropriate.

Changes of note

  • This PR changes the visited link style for all anchors, with the exception anchors of Stacks components that should not (see this change for the list of unaffected elements). I'll be testing this in Core, but would appreciate feedback on this approach in the meantime.
  • I've applied styles based on the LVHA-order (:link:visited:hover:active). See the :visited MDN page for more details

Testing these changes

Tip

The below seems to not work in incognito/private browser modes since (TIL) these modes do not honor visited link styles (at least in Mac Firefox and Chrome)

  1. Visit the link component docs page
  2. Observe the color of the "Visited" link (this one has forced visited style)
  3. Go to https://deploy-preview-1735--stacks.netlify.app//product/components/links/# (note the # at the end. Observe that the appropriate links are now purple-500. Ensure this renders as expected in all modes.
  4. Visit other components that include links to ensure they only gain this purple-500 when expected

Copy link

netlify bot commented May 14, 2024

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 6cc395e
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/6647d3bf370643000895111e
😎 Deploy Preview https://deploy-preview-1735--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@CGuindon CGuindon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just me or is there no more hover color change on the Visited link? I think it use to go up/down a stop depending on dark/light mode. I'm looking at the Stacks docs now and in the PR env here

@giamir
Copy link
Contributor

giamir commented May 16, 2024

Is it just me or is there no more hover color change on the Visited link? I think it use to go up/down a stop depending on dark/light mode. I'm looking at the Stacks docs now and in the PR env here

I can reproduce the same. It looks like now visited links are always purple-500 no matter if hovered on or not.
Before they went from theme-secondary-500 (visited, not hovered) to theme-secondary-400 (visited, hovered). The figma comp doesn't specify what to use for visited+hover status though.

Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not expect this "simple" ticket to involve so many changes. 😱

Apart from the point raised from @CGuindon the other changes seems reasonable to me. I feel we are wrestling quite a bit CSS specificity (hence the many changes). I also do think there is a high probability we might have missed something but it is also a relatively low risk change so I am ok with going ahead with it.

Thanks @dancormier for the thorough PR.

lib/components/breadcrumbs/breadcrumbs.less Outdated Show resolved Hide resolved
@CGuindon
Copy link
Collaborator

I would keep the same logic of color stops purple-400 for visited, hovered

I'd like to punt on implementing visited styles for links within `.s-input-message` since the link colors do not follow our standard 400/500/600 pattern for default/hover/visited. @CGuindon thoughts?
@dancormier
Copy link
Contributor Author

I would keep the same logic of color stops purple-400 for visited, hovered

I've added purple-400 for visited+hovered links in 20f10fb

input message

I noticed tests were failing for the s-input-message component and realized that the visited link style was being applied to the link within that element (for some reason, our visual tests render those links as if they're visited no matter what 🤷‍♂️). I found that we use a different color pattern than our usual 400/500/600 pattern for default/hover/visited.

@CGuindon would we be okay to punt on visited link styles for anchors within .s-input-message? I can add it but I'm hesitant to introduce more complication at this moment.

@dancormier dancormier marked this pull request as ready for review May 16, 2024 22:47
@CGuindon
Copy link
Collaborator

@dancormier

  1. weird that the the s-input-message component automatically uses visited links — but that's a problem for another day
  2. Ideally these remain the colors they are now — warning message link stays red. If we punt on this component, would those links be purple or would they stay as they are now?

@dancormier
Copy link
Contributor Author

@CGuindon

1. weird that the [the s-input-message component](https://deploy-preview-1735--stacks.netlify.app/product/components/inputs/#validation-examples) automatically uses visited links — but that's a problem for another day

To be clear, the links only show up as visited in the visual tests images, not by default when the component is rendered.

2. Ideally these remain the colors they are now — warning message link stays red. If we punt on this component, would those links be purple or would they stay as they are now?

They would stay as they are now (visited state will not change the color of the link within the input messages)

@CGuindon
Copy link
Collaborator

I can see in the inspector that _li-fc-hover-visited: is set to purple-400 but when I hover, the color isn't changing. Checking this in the PR env.

Also... (and this is my fault for not catching before) but since we are going darker for blue links on hover:

  • default blue-400
  • hover blue-500

shouldn't we go darker for purple as well on hover?

  • visited default purple-500
  • visited hover-purple-600
Screenshot 2024-05-17 at 10 57 07 AM

@dancormier
Copy link
Contributor Author

@CGuindon @giamir I decided to ditch the :not(.s-badge):not(… approach I was taking before in 2865716 and instead go with what's in aecad2a.

This is the most significant change:

a {
    &:visited {
        &:not([class*="s-"]),
        &.s-link,
        &.s-sidebarwidget--action,
        &.s-user-card--link {
            &:hover {
                color: var(--_li-fc-hover-visited);
            }

            color: var(--_li-fc-visited);
        }
    }
}

This is specifying to apply these styles to anchors that don't include a class starting with s-, with the exception of s-link, s-sidebarwidget--action, or s-user-card--link. IMO, this is way safer and simpler than the previous because it no longer requires this long list of :not exclusions that increases the specificity of the rule.

@giamir I'd like to get your thoughts on this approach and it's relative safety when compared to the previous approach when you have a moment.


The stuff below is nonsense, so feel free to ignore it.

Story time (aka TIL about properties allowed in :visited)

The big problem was with specificity. If we applied styles to a:visited, most visited links would gain that style even if we have other non-visited styles on that element (think a .s-btn on an anchor). This is not what we want.

I had a lightbulb moment: what if we specificy CSS variables on a:visited so the variable is a high specificity but the style being applied would stay only on the a selector so it could be easily overwritten.

a,
.s-link {
  --_li-fc: var(--theme-link-color, var(--theme-secondary-400));
  …

  &:visited {
    --_li-fc-override: var(--purple-500);
  }

  color: var(--_li-fc-override, var(--_li-fc));
  …
}

This seemed like a winner. I implemented it and was… very confused. The CSS variable was being picked up by the browser. The style said it was applied in the inspector. But, when I looked at the page, the color defined as a CSS variable in :visited wasn't applying. What's the deal?

The TIL

You can only set specific CSS properties within :visited for security/privacy reasons 🙃. MDN gives more details, but for this case the big takeaway is:

Tip

You cannot define or redefine CSS variables within the :visited selector.

C'mon now, CSS!

Copy link
Collaborator

@CGuindon CGuindon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see all the purples now 🎉!

Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giamir I'd like to get your thoughts on this approach and it's relative safety when compared to the previous approach when you have a moment.

I think &:not([class*="s-"]) is pretty smart and not less safe than what we had before. Great work @dancormier! 🎉

lib/components/button/button.less Show resolved Hide resolved
@dancormier dancormier merged commit fdc320d into develop May 20, 2024
11 checks passed
@dancormier dancormier deleted the STACKS-481/visited-link-color-updates branch May 20, 2024 14:06
dancormier added a commit that referenced this pull request May 24, 2024
dancormier added a commit that referenced this pull request May 24, 2024
* fix(links): make visited dark mode links purple-500

* Update color for .s-link__visited

* Fix link visited selector, anchor visited selector

* minor formatting fix

* Prevent changes from impacting input messages

I'd like to punt on implementing visited styles for links within `.s-input-message` since the link colors do not follow our standard 400/500/600 pattern for default/hover/visited. @CGuindon thoughts?

* Implement style for hover + visited links

* formatting

* Use purple-600 for visited+hover

* exclude all `.s-*` elements from default visited styles (with exception)

* fix typo

* Revert unneeded breadcrumb visited override

* whitespace fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants