Skip to content

Conversation

@joshblack
Copy link
Member

In our maintainer sync today, it came up that we no longer need to polyfill focus-visible based on GitHub's browser support range. This PR removes this polyfill and any references to the explicit .focus-visible class

Changelog

New

Changed

  • Instances of .focus-visible to just :focus-visible

Removed

  • The focus-visible polyfill

Rollout strategy

  • Minor release

I selected a minor release here but could also see this considered a breaking change since we don't explicitly call out our browser compatability (unless we indicate that it should match GitHub's?)

Testing & Reviewing

  • This PR changes some BaseStyles so we should confirm that none of these impact major functionality
  • In most cases, this PR removes CSS selectors which had a fallback already with :focus-visible. We should make sure no remaining .focus-visible selectors remain or are in use

@joshblack joshblack requested review from a team and siddharthkp January 22, 2024 17:03
@changeset-bot
Copy link

changeset-bot bot commented Jan 22, 2024

🦋 Changeset detected

Latest commit: 9cb86bc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@joshblack joshblack changed the title Refactor/remove focus visible polyfill refactor(project): drop focus-visible polyfill Jan 22, 2024
Copy link
Contributor

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

🔥 🔥

@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
dist/browser.esm.js 103.67 KB (-0.77% 🔽)
dist/browser.umd.js 104.28 KB (-0.77% 🔽)

@TylerJDev
Copy link
Member

I think this might affect the changes that could happen in #4166. Does this mean we'd need to find a new solution to fix the referenced bug? 🤔

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

🎉

Let's test this with dotcom to be extra extra sure! For example, do these 3 instances depend on primer brining the polyfill?

I selected a minor release here but could also see this considered a breaking change since we don't explicitly call out our browser compatability (unless we indicate that it should match GitHub's?)

Happy with minor

@joshblack
Copy link
Member Author

Closing as we currently need this polyfill for some odd Safari behavior 🤔

@joshblack joshblack closed this Feb 8, 2024
@joshblack joshblack deleted the refactor/remove-focus-visible-polyfill branch February 8, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants