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

Improve contrast for icons in invisible button on hover in dark mode #2250

Merged
merged 27 commits into from
Sep 26, 2023

Conversation

langermank
Copy link
Contributor

@langermank langermank commented Sep 19, 2023

Authors: Please fill out this form carefully and completely.

Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.

What are you trying to accomplish?

  • Update Button CSS to use new tokens for icon colors in the invisible variant
  • Sneaking in a fix for v8 colors to remove the unused inset box-shadow on primary buttons

Screenshots

Before After
icon button hover icon button hover

Integration

  • Bump Primitives to the latest version

List the issues that this change affects.

Closes https://github.com/github/accessibility-audits/issues/5764

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

Accessibility

  • Fixes axe scan violation - This change fixes an existing axe scan violation.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • 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 a review from a team as a code owner September 19, 2023 21:52
@langermank langermank requested review from a team, lukasoppermann and camertron September 19, 2023 21:52
@changeset-bot
Copy link

changeset-bot bot commented Sep 19, 2023

🦋 Changeset detected

Latest commit: 2867f9e

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

This PR includes changesets to release 1 package
Name Type
@primer/view-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 github-actions bot added the css Pull requests that update CSS code label Sep 19, 2023
@langermank langermank temporarily deployed to preview September 19, 2023 21:54 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2023

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

@langermank langermank marked this pull request as draft September 19, 2023 22:10
@langermank langermank temporarily deployed to preview September 20, 2023 20:26 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to preview September 20, 2023 21:45 — with GitHub Actions Inactive
@langermank langermank marked this pull request as ready for review September 20, 2023 22:44
@langermank langermank temporarily deployed to preview September 20, 2023 22:44 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to preview September 21, 2023 00:06 — with GitHub Actions Inactive
package.json Outdated
@@ -62,7 +62,7 @@
"@github/prettier-config": "0.0.4",
"@playwright/test": "^1.35.1",
"@primer/css": "21.0.2",
"@primer/primitives": "7.12.0",
"@primer/primitives": "0.0.0-20230921000240",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we fix this to a release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I need to make another patch release before I merge this PR

@langermank langermank temporarily deployed to preview September 25, 2023 15:04 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to preview September 25, 2023 16:28 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to preview September 25, 2023 17:36 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to preview September 25, 2023 18:05 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to preview September 25, 2023 21:10 — with GitHub Actions Inactive
@camertron camertron temporarily deployed to preview September 26, 2023 03:16 — with GitHub Actions Inactive
@camertron camertron temporarily deployed to preview September 26, 2023 05:48 — with GitHub Actions Inactive
@camertron camertron temporarily deployed to preview September 26, 2023 06:32 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to preview September 26, 2023 14:41 — with GitHub Actions Inactive
@langermank langermank merged commit 66c4dd6 into main Sep 26, 2023
@langermank langermank deleted the invisible-button-icon-hover-color branch September 26, 2023 17:43
@langermank langermank temporarily deployed to preview September 26, 2023 17:43 — with GitHub Actions Inactive
@primer primer bot mentioned this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css Pull requests that update CSS code patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants