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

Remove deprecated focus style primitives #2510

Merged
merged 9 commits into from
Nov 3, 2022

Conversation

langermank
Copy link
Contributor

Describe your changes here.

Removing old/unused focus style CSS vars so we can deprecate them from primer/primitives

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@langermank langermank requested review from a team and colebemis November 1, 2022 17:52
@changeset-bot
Copy link

changeset-bot bot commented Nov 1, 2022

🦋 Changeset detected

Latest commit: ec35a70

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 Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 78.84 KB (-0.04% 🔽)
dist/browser.umd.js 79.49 KB (-0.02% 🔽)

@langermank langermank temporarily deployed to github-pages November 1, 2022 18:05 Inactive
@langermank langermank temporarily deployed to github-pages November 1, 2022 19:11 Inactive
@joshblack
Copy link
Member

@langermank random question, would there be a good way to pull out common styles (like for focus in this PR and defined in https://github.com/github/primer/issues/409) into a common module to apply to components when we need them? Similar to helper classes, mixins, or functions but with styled-components.

@langermank
Copy link
Contributor Author

@langermank random question, would there be a good way to pull out common styles (like for focus in this PR and defined in github/primer#409) into a common module to apply to components when we need them? Similar to helper classes, mixins, or functions but with styled-components.

@joshblack I think ideally we would put global focus styles in this file and then create some shared "mixins" so we can have custom focus style overrides for components. This is what we've done in PCSS. To be honest I'm not totally sure what the state of focus styles is in PRC, as I bet we are actually seeing focus styles in memex coming from PCSS. In that BaseStyles file, its removing outline globally 😦 I'd be happy to follow up and work on that, but this PR is just an attempt to remove the older focus style CSS vars 😆

@langermank langermank temporarily deployed to github-pages November 1, 2022 19:35 Inactive
@langermank
Copy link
Contributor Author

@joshblack I just stumbled upon this: https://github.com/primer/react/blob/main/src/_getGlobalFocusStyles.ts

@langermank langermank temporarily deployed to github-pages November 3, 2022 18:28 Inactive
@langermank langermank temporarily deployed to github-pages November 3, 2022 20:59 Inactive
@joshblack joshblack temporarily deployed to github-pages November 3, 2022 21:09 Inactive
@langermank langermank temporarily deployed to github-pages November 3, 2022 22:47 Inactive
@langermank langermank merged commit c326777 into main Nov 3, 2022
@langermank langermank deleted the remove-deprecated-focus-styles branch November 3, 2022 23:03
@primer-css primer-css mentioned this pull request Nov 3, 2022
broccolinisoup pushed a commit that referenced this pull request Nov 3, 2022
* remove deprecated primitives

* Create afraid-wombats-drive.md

* snippy snaps

* move to draft directory

* fix focus color

* test: update snapshots

Co-authored-by: Josh Black <josh@josh.black>
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.

None yet

2 participants