facebookincubator / infima

A UI framework that provides websites with the minimal CSS and JS needed to get started with building a modern responsive beautiful website
https://infima.dev
MIT License
408 stars 55 forks source link

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

Closed slorber closed 2 years ago

slorber commented 2 years ago

Revert half of https://github.com/facebookincubator/infima/pull/266

a:any-link:hover change is not necessary and was based on an artificial usage of <a className="xyz"> which we don't have in practice (we use <span> in such cases)


Former issue text before reverting Fix issue where all links have unexpected underlines: CleanShot 2022-07-13 at 12 20 00@2x Docusaurus issue: https://github.com/facebook/docusaurus/issues/7748 See comment https://github.com/facebookincubator/infima/pull/266/files#r917208453 Alternative PR: https://github.com/facebookincubator/infima/pull/267 Support for `:where` is good enough and we already use it anyway: https://caniuse.com/?search=where It's a good new tool to keep CSS specificity low
netlify[bot] commented 2 years ago

Deploy Preview for infima ready!

Name Link
Latest commit 0de9584f843669783a5df76dbeb4aaa5d68c702e
Latest deploy log https://app.netlify.com/sites/infima/deploys/62cea7b7d2b0ca00097fa9ee
Deploy Preview https://deploy-preview-268--infima.netlify.app/demo
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 settings.

github-actions[bot] commented 2 years ago

Size Change: -3.34 kB (-1%)

Total Size: 558 kB

Filename Size Change
./packages/core/dist/css/default-dark/default-dark-rtl.css 80.5 kB -500 B (-1%)
./packages/core/dist/css/default-dark/default-dark-rtl.min.css 57.4 kB -334 B (-1%)
./packages/core/dist/css/default-dark/default-dark.css 80.5 kB -500 B (-1%)
./packages/core/dist/css/default-dark/default-dark.min.css 57.3 kB -334 B (-1%)
./packages/core/dist/css/default/default-rtl.css 78.4 kB -500 B (-1%)
./packages/core/dist/css/default/default-rtl.min.css 56.2 kB -334 B (-1%)
./packages/core/dist/css/default/default.css 78.4 kB -500 B (-1%)
./packages/core/dist/css/default/default.min.css 56.2 kB -334 B (-1%)
â„đïļ View Unchanged | Filename | Size | | :--- | :---: | | `./packages/core/dist/js/alerts.js` | 409 B | | `./packages/core/dist/js/alerts.min.js` | 409 B | | `./packages/core/dist/js/button-groups.js` | 267 B | | `./packages/core/dist/js/button-groups.min.js` | 267 B | | `./packages/core/dist/js/dropdowns.js` | 1.01 kB | | `./packages/core/dist/js/dropdowns.min.js` | 1.01 kB | | `./packages/core/dist/js/menu.js` | 2.4 kB | | `./packages/core/dist/js/menu.min.js` | 2.4 kB | | `./packages/core/dist/js/navbar.js` | 1.08 kB | | `./packages/core/dist/js/navbar.min.js` | 1.08 kB | | `./packages/core/dist/js/pills.js` | 270 B | | `./packages/core/dist/js/pills.min.js` | 270 B | | `./packages/core/dist/js/radio-behavior.js` | 705 B | | `./packages/core/dist/js/radio-behavior.min.js` | 705 B | | `./packages/core/dist/js/tabs.js` | 267 B | | `./packages/core/dist/js/tabs.min.js` | 267 B |

compressed-size-action

github-actions[bot] commented 2 years ago

Dist CSS Diff

diff --git a/packages/core/dist/css/default/default.css b/packages/core/dist-branch/css/default/default.css
index f258e29..52e3ab6 100644
--- a/packages/core/dist/css/default/default.css
+++ b/packages/core/dist-branch/css/default/default.css
@@ -1233,25 +1233,7 @@ a {
   transition: color var(--ifm-transition-fast) var(--ifm-transition-timing-default);
 }

-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 {
+a:hover {
     color: var(--ifm-link-hover-color);
     /* autoprefixer: ignore next */
     text-decoration: var(--ifm-link-hover-decoration);
Josh-Cena commented 2 years ago

Thought about it, but we may expect more complaints like https://github.com/facebook/docusaurus/issues/7617 coming in 😅 I don't have objections to this, but I wonder what the purpose of changing the style for all a elements in https://github.com/facebookincubator/infima/pull/266 was?

Edit: I see this:

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.

Not sure if a11y experts will accept the idea of "non-link <a> tags" at all. In Docusaurus we render it as a <span> if there's no href. What about just reverting this change?

slorber commented 2 years ago

Thought about it, but we may expect more complaints like https://github.com/facebook/docusaurus/issues/7617 coming in 😅

Looks like it may not be a big deal in this specific case if some users have an inappropriate :hover effect.

I don't have objections to this but I wonder what the purpose of changing the style for all a elements in https://github.com/facebookincubator/infima/pull/266 was?

Yes, I think it may not be useful in the first place 😅 thanks for noticing that

The created demo looks artificial and does not match what we have on the Docusaurus side.

I've run this query on many Docusaurus pages and couldn't find a single usage: document.querySelectorAll("a:not(:any-link)") (and ensured the query actually works with a manual test for sure)

@srmagura I'm going to revert this change and adapt the "artificial" example ensure that breadcrumbs without links are not using <a> without href

slorber commented 2 years ago

If the a element has no href attribute, then the element represents a placeholder for where a link might otherwise have been placed, if it had been relevant.

https://www.w3.org/TR/2011/WD-html5-author-20110809/the-a-element.html

I guess we can assume we'll never have this case in the future, but may revisit later the introduction of :where() if needed, as it still makes sense technically ðŸĪ·â€â™‚ïļ

Wonder if the breadcrumb items without links can be interpreted as "placeholder" ðŸĪŠ

Note we have Infima demo items using <a> without links so I'll also clean these up to match Docusaurus HTML structure

Josh-Cena commented 2 years ago

Maybe a11y gurus like @bendmyers has more insight on this 😄

We do have the jsx-a11y rule that forbids this case:

The href attribute is required for an anchor to be keyboard accessible. Provide a valid, navigable address as the href value. If you cannot provide an href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md

slorber commented 2 years ago

Will merge this for now, looks good enough but let's continue the discussion

Note that we have some cases of <a href="#"> in Docusaurus too, wonder if those are legit usages

document.querySelectorAll("a[href='#']")

CleanShot 2022-07-13 at 13 23 27@2x
srmagura commented 2 years ago

Thanks @slorber !!! Seems like I made this change based on the Infima demo site having an example which does not actually occur in practice. Sorry for the regression.