-
Notifications
You must be signed in to change notification settings - Fork 81
Remove :hover:visited styles and decrease base link specificity (Fix #1071) #1077
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
Conversation
74df85d to
35fa5bf
Compare
| .mzp-c-button, | ||
| a.mzp-c-button:link, | ||
| a.mzp-c-button:visited { | ||
| a.mzp-c-button { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The increased specificity was added in #1045 to address the problem where links were styled like buttons and then got focus.
This has been solved by removing the text colour change for :focus styles from the default :link styles.
aaf6ce2 to
aa7d62f
Compare
| color: $link-color; | ||
| text-decoration: underline; | ||
|
|
||
| &:visited { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:visited moves earlier in the source so is over-ridden by the hover, focus, and active styles that follow. We rely on the cascade to style a link when it's hovered instead of explicitly declaring :visited:hover styles.
| } | ||
|
|
||
| &:hover, | ||
| &:focus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declaring the colour change on focus was the root problem in #842
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could split the definition and keep the text-decoration styles for focus if we wanted but we also have the focus ring to indicate state.
0063001 to
14709d3
Compare
14709d3 to
1967e7e
Compare
reduce specificity in components
1967e7e to
9e81e70
Compare
This comment was marked as outdated.
This comment was marked as outdated.
843e2ba to
43d5886
Compare
| color: $link-color; | ||
| text-decoration: underline; | ||
|
|
||
| &:where(:visited) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't decrease the specificity enough for :visited with re-arranging things alone.
I think using :where only on the visited links is good progressive enhancement. Older browsers will still be able to see and interact with links without any problem. Newer browsers get the visual indication of visited as well.
Maybe this is the best of both worlds from @maureenlholland's #1054
maureenlholland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc 🎉 this is looking much cleaner - nothing blocking: minor typo and a suggestion on possibly making the button component selector even simpler
tested on moz and fxc pages in bedrock (thanks for the disable redirect tip), fixes the color contrast issues seen on manifesto button and account promo links (i.e. on http://localhost:8000/fr/firefox/browsers/mobile/)
| color: $link-color; | ||
| text-decoration: underline; | ||
|
|
||
| &:where(:visited) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking) Might be worth putting the comment in code so it's clear why :where is only used in this case
| a.mzp-c-button:link, | ||
| a.mzp-c-button:visited { | ||
| .mzp-c-button, /* stylelint-disable-line no-duplicate-selectors */ | ||
| a.mzp-c-button { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): in the interest of keeping specificity as low as possible, is it possible to remove the prepended a selector too? the button class should overrule the base element
| a.mzp-c-button { | |
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of :where() on link visited state reduces specificity to element selector
We should be able to rely solely on .mzp-c-button class selector for button styles (regardless of whether underlying element is button or link). Link element styles only use color, background-color, and text-decoration properties, so we could remove the prepended a from Button shape and style selector too (L76)
| $link-color-hover-inverse: tokens.$color-blue-05; | ||
| $link-color-hover: tokens.$color-link-hover; | ||
| $link-color-inverse: tokens.$color-blue-10; | ||
| $link-color-visited-hover-inverse: tokens.$color-violet-05; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 🧹 🧹
Co-authored-by: maureenlholland <[email protected]>
Description
:hover:visitedstyles for links:focusfor links (rely on focus ring instead)CHANGELOG.md.Issue
Fix #1071
Testing
Toggle through the various states using dev tools and make sure they're all displaying as intended.
http://localhost:3000/components/detail/links--default
http://localhost:3000/components/detail/button--primary
http://localhost:3000/components/detail/footer
http://localhost:3000/components/detail/notification-bar--warning
Hard-swap the files into bedrock and see if it breaks anything (if you turn off the Firefox redirects you can view Fxc content and skip testing springfield 🙂 )
It should fix the :focus state for buttons on this page: http://localhost:3000/en-US/about/manifesto/