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

Use open/closed colors for StateLabel #3041

Merged
merged 10 commits into from
Mar 24, 2023
Merged

Use open/closed colors for StateLabel #3041

merged 10 commits into from
Mar 24, 2023

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Mar 16, 2023

This is a follow-up to https://github.com/github/primer/issues/725 and replaces the success/danger colors with open/closed colors for StateLabel. See ADR for reasoning. Note that only color blind themes are affected.

Closes #3038

Screenshots

Please provide before/after screenshots for any visual changes

Before After
blue background orange background
blue background orange background
orange background gray background

This should match PVC's State component for the color blind themes:

Screen Shot 2023-03-16 at 11 33 41

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.

@changeset-bot
Copy link

changeset-bot bot commented Mar 16, 2023

🦋 Changeset detected

Latest commit: 968ce29

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 Mar 16, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 94.6 KB (-0.01% 🔽)
dist/browser.umd.js 95.22 KB (0%)

@github-actions github-actions bot temporarily deployed to storybook-preview-3041 March 16, 2023 02:41 Inactive
@simurai simurai temporarily deployed to github-pages March 16, 2023 02:44 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3041 March 16, 2023 02:45 Inactive
@primer primer bot temporarily deployed to github-pages March 16, 2023 03:38 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3041 March 16, 2023 03:38 Inactive
@simurai simurai marked this pull request as ready for review March 16, 2023 06:54
@simurai
Copy link
Contributor Author

simurai commented Mar 16, 2023

@joshblack I added the update snapshots label and it updated the snapshots with 8865a6a but the vrt/aat checks are still failing. Is there another step to tell that the new snapshots are the new and correct ones?

@joshblack joshblack temporarily deployed to github-pages March 16, 2023 16:57 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3041 March 16, 2023 16:58 Inactive
@joshblack joshblack temporarily deployed to github-pages March 16, 2023 17:42 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3041 March 16, 2023 17:43 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3041 March 16, 2023 17:56 Inactive
@joshblack
Copy link
Member

Hey @simurai! Thanks for bringing this up, fixed a bug with how the snapshots were being uploaded to make sure the missing ones were brought in 👀

For the a11y violations, are those contrast violations expected? If so, happy to batch them up with the other tests where we are ignoring them but just wanted to double-check with you first 👍

@primer primer bot temporarily deployed to github-pages March 16, 2023 18:03 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3041 March 16, 2023 18:03 Inactive
@joshblack joshblack temporarily deployed to github-pages March 16, 2023 18:56 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3041 March 16, 2023 18:56 Inactive
@primer primer bot temporarily deployed to github-pages March 16, 2023 19:43 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3041 March 16, 2023 19:44 Inactive
@simurai
Copy link
Contributor Author

simurai commented Mar 17, 2023

@joshblack For the a11y violations, are those contrast violations expected? If so, happy to batch them up with the other tests where we are ignoring them but just wanted to double-check with you first 👍

Ahh.. yes. I can see the contrast failing in Storybook too.

Screen Shot 2023-03-17 at 15 14 02

There are potential solutions, but that's something that would need to be fixed in Primitives.

Is there a way to add an exception for now?

@joshblack
Copy link
Member

@simurai definitely! Just pushed something up for it. For any tests that we want to ignore color contrast, we can omit the options passed to toHaveNoViolations() and contrast by default should not be included as a check 👍

@simurai
Copy link
Contributor Author

simurai commented Mar 22, 2023

Just pushed something up for it.

Thanks. 🙇 All checks pass now. 👍

@simurai
Copy link
Contributor Author

simurai commented Mar 22, 2023

@joshblack Just pushed something up for it.

All checks pass now. 👍 Thanks. 🙇

@simurai simurai added this pull request to the merge queue Mar 24, 2023
Merged via the queue into main with commit e5f1330 Mar 24, 2023
@simurai simurai deleted the fix-StateLabel branch March 24, 2023 06:08
@primer-css primer-css mentioned this pull request Mar 24, 2023
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.

Use correct colors for StateLabel
2 participants