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

Add focus and hover colors to all themes in Item using functional variables #1486

Merged
merged 10 commits into from
Nov 22, 2021

Conversation

pksjce
Copy link
Collaborator

@pksjce pksjce commented Sep 30, 2021

Before this PR, hardcoded RGB colors were being used for the hover and focus states. I changed them to use the functional color system as described in #1480

Screenshots

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.

@pksjce pksjce requested review from a team and colebemis September 30, 2021 05:43
@changeset-bot
Copy link

changeset-bot bot commented Sep 30, 2021

🦋 Changeset detected

Latest commit: e1208d4

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

This PR includes changesets to release 1 package
Name Type
@primer/components 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 Sep 30, 2021

size-limit report 📦

Path Size
dist/browser.esm.js 55.96 KB (-0.62% 🔽)
dist/browser.umd.js 56.38 KB (-0.58% 🔽)

@pksjce
Copy link
Collaborator Author

pksjce commented Sep 30, 2021

Notes -

  1. hover -> neutral.subtle
  2. focus -> neutral.muted
    Would do well with some guidance from @auareyou here

@colebemis
Copy link
Contributor

Looks like neutral/danger.subtle for the hover state and neutral/danger.muted for the focus state work in most cases. The exception is the focus state of the danger variant in dark high contrast:

CleanShot 2021-09-30 at 08 38 05

danger.muted and danger.emphasis are the same in dark high contrast which makes the danger.fg text invisible.

Let's loop in @tallys and @auareyou because I think this issue is going to require more design work than I initially thought.

@mperrotti
Copy link
Contributor

@pksjce - should we modify this PR now that primer/primitives#256 has merged?

@@ -418,8 +391,6 @@ export const Item = React.forwardRef((itemProps, ref) => {
data-id={id}
onKeyPress={keyPressHandler}
onClick={clickHandler}
hoverBackground={disabled ? 'inherit' : hoverBackground}
Copy link
Member

Choose a reason for hiding this comment

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

this feels so good!

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.

(added a word for the automated changelog)

Looks good, let's ship it!

Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

🚢 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants