fix: breadcrumb links without `href` should not have hover styles by srmagura · Pull Request #266 · facebookincubator/infima

srmagura · 2022-06-25T18:54:48Z

Breadcrumb items do not necessarily have an href, but they always change color when the user hovers over them. The hover styles give the impression that the breadcrumb item is a clickable link even if this is not the case.

This can be seen in the Redux documentation — neither "Tutorials" nor "Redux Essentials" are links, yet they have hover styles.

This PR uses the :link pseudoselector to fix this.

Edit: Signed the CLA.

facebook-github-bot · 2022-06-25T18:54:52Z

Hi @srmagura!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

netlify · 2022-06-25T18:56:23Z

✅ Deploy Preview for infima ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`b29d289`
🔍 Latest deploy log	https://app.netlify.com/sites/infima/deploys/62c43b2fdd602700083545b9
😎 Deploy Preview	https://deploy-preview-266--infima.netlify.app/demo
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

facebook-github-bot · 2022-06-25T20:16:37Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

slorber

Agree this UX is not ideal and we should change it.

Breadcrumb items do not necessarily have an href, but they always change color when the user hovers over them.

In practice it's more:

breadcrumb items might be simple <span> instead of <a> when we don't have a link

Not 100% sure using :link is the solution here?

Also worth adding a test to our demo page here: https://deploy-preview-266--infima.netlify.app/demo/

We only have breadcrumb items with links currently (unlike what we have on the Docusaurus side in practice). Please try to add something here that helps reviewing the current PR.

slorber · 2022-06-30T08:22:46Z

    @mixin transition background color;

-    &:hover {
+    &:link:hover {


The :link CSS pseudo-class represents an element that has not yet been visited.

Does it mean that we'll lose the hover effect once the link is visited? 😅

Maybe using :is(a):hover would be better? (90% support good enough for this usecase)

TIL there's also an :any-link pseudo-selector, looks like a potential good fit?

Great tip regarding :any-link. I had no idea :link was only for unvisited links — that's really poor design.

yes, not very intuitive 😅

slorber · 2022-06-30T08:28:36Z

  @mixin transition color;

-  &:hover {
+  &:link:hover {


🤷‍♂️ this one looks more general and not sure it's directly related to your problem, considering the element is already a <a> there?

Can it have unwanted side effects? + not sure :link is what we want anyway

You can have <a> elements that don't represent links (like headings). It usually only makes sense to display hover styles if the <a> represents a link.

This change was also necessary to prevent the color of the breadcrumb item from changing when hovered.

I see, what I understand is we don't want the text color to change on hover in this demo:

https://deploy-preview-266--infima.netlify.app/demo/#breadcrumbs

srmagura · 2022-07-05T13:22:45Z

Thanks for the feedback. I switched to :any-link and added an example.

breadcrumb items might be simple instead of when we don't have a link

True, but I assume you would not want hover styles if the breadcrumb item is a <span>, correct?

Josh-Cena · 2022-07-05T13:38:22Z

diff --git a/packages/core/dist/css/default/default.css b/packages/core/dist-branch/css/default/default.css
index 95fc6e3..f258e29 100644
--- a/packages/core/dist/css/default/default.css
+++ b/packages/core/dist-branch/css/default/default.css
@@ -1233,[7](https://github.com/facebookincubator/infima/runs/7197269101?check_suite_focus=true#step:4:7) +1233,25 @@ a {
   transition: color var(--ifm-transition-fast) var(--ifm-transition-timing-default);
 }

-a:hover {
+a:link:hover, a:visited:hover {
+    color: var(--ifm-link-hover-color);
+    /* autoprefixer: ignore next */
+    text-decoration: var(--ifm-link-hover-decoration);
+  }
+
+a:-webkit-any-link:hover {
+    color: var(--ifm-link-hover-color);
+    /* autoprefixer: ignore next */
+    text-decoration: var(--ifm-link-hover-decoration);
+  }
+
+a:-moz-any-link:hover {
+    color: var(--ifm-link-hover-color);
+    /* autoprefixer: ignore next */
+    text-decoration: var(--ifm-link-hover-decoration);
+  }
+
+a:any-link:hover {
     color: var(--ifm-link-hover-color);
     /* autoprefixer: ignore next */
     text-decoration: var(--ifm-link-hover-decoration);
@@ -172[8](https://github.com/facebookincubator/infima/runs/7197269101?check_suite_focus=true#step:4:8),7 +1746,22 @@ hr {
     transition-timing-function: var(--ifm-transition-timing-default);
   }

-.breadcrumbs__link:hover {
+.breadcrumbs__link:link:hover, .breadcrumbs__link:visited:hover, area.breadcrumbs__link[href]:hover {
+      background: var(--ifm-breadcrumb-item-background-active);
+      text-decoration: none;
+    }
+
+.breadcrumbs__link:-webkit-any-link:hover {
+      background: var(--ifm-breadcrumb-item-background-active);
+      text-decoration: none;
+    }
+
+.breadcrumbs__link:-moz-any-link:hover {
+      background: var(--ifm-breadcrumb-item-background-active);
+      text-decoration: none;
+    }
+
+.breadcrumbs__link:any-link:hover {
       background: var(--ifm-breadcrumb-item-background-active);
       text-decoration: none;
     }

srmagura · 2022-07-05T13:44:02Z

@Josh-Cena Why are those changes necessary? I tested in Chrome, Safari, and Firefox and the vendor prefixes were not necessary.

Josh-Cena · 2022-07-05T13:45:57Z

Ah, no, that's the "post diff comment" workflow's result 😄 It can't be posted on PRs created from forks, so I manually post it so we know how the distributed CSS has changed. See #255 (comment) for example.

srmagura · 2022-07-05T13:47:41Z

Ooooohhhhh, OK. Do I need to do anything to get that workflow to pass?

Josh-Cena · 2022-07-05T13:50:13Z

No, it's a problem with our workflow plus some permission quirks. It's mostly okay this way.

slorber · 2022-07-06T16:12:50Z

LGTM thanks 👍

pranabdas · 2022-07-09T01:46:27Z

+  &:any-link:hover {
    color: var(--ifm-link-hover-color);
    /* autoprefixer: ignore next */
    text-decoration: var(--ifm-link-hover-decoration);


I guess this is causing text-decoration: underline for all links including buttons, sidebar menu, navbar, navbar dropdown menu. facebook/docusaurus#7748

Adding the any-link pseudo selector increases the specificity, now a:any-link:hover is more specific than other selectors like button:hover

Thanks for pointing this out.

#267

Make breadcrumb links without href not have hover styles

ff141ec

srmagura mentioned this pull request Jun 25, 2022

Breadcrumb links don't work markerikson/react-community-tools-practices-cheatsheet#23

Closed

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 25, 2022

slorber requested changes Jun 30, 2022

View reviewed changes

Use :any-link instead of :link. Add example

b29d289

srmagura requested a review from slorber July 5, 2022 13:23

slorber changed the title ~~Make breadcrumb links without href not have hover styles~~ fix: breadcrumb links without href should not have hover styles Jul 6, 2022

slorber merged commit 4cc2ff3 into facebookincubator:main Jul 6, 2022

pranabdas reviewed Jul 9, 2022

View reviewed changes

srmagura mentioned this pull request Jul 9, 2022

Fix text-decoration regression #267

Closed

Josh-Cena mentioned this pull request Jul 13, 2022

fix: revert useless a:any-link:hover change #268

Merged

Conversation

srmagura commented Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 25, 2022

Action Required

Process

Uh oh!

netlify Bot commented Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for infima ready!

Uh oh!

facebook-github-bot commented Jun 25, 2022

Uh oh!

slorber left a comment

Choose a reason for hiding this comment

Uh oh!

slorber Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

srmagura Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

slorber Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

slorber Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

srmagura Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slorber Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

srmagura commented Jul 5, 2022

Uh oh!

Josh-Cena commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srmagura commented Jul 5, 2022

Uh oh!

Josh-Cena commented Jul 5, 2022

Uh oh!

srmagura commented Jul 5, 2022

Uh oh!

Josh-Cena commented Jul 5, 2022

Uh oh!

slorber commented Jul 6, 2022

Uh oh!

pranabdas Jul 9, 2022

Choose a reason for hiding this comment

Uh oh!

yangshun Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srmagura Jul 9, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

srmagura commented Jun 25, 2022 •

edited

Loading

netlify Bot commented Jun 25, 2022 •

edited

Loading

srmagura Jul 5, 2022 •

edited

Loading

Josh-Cena commented Jul 5, 2022 •

edited

Loading

yangshun Jul 9, 2022 •

edited

Loading