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

Fix label colors for border and text #2487

Merged
merged 3 commits into from
Oct 28, 2022

Conversation

lukasoppermann
Copy link
Contributor

Label colors for border and text were mixed up for variants done and sponsors.

This leads to an a11y issue were text colors don't pass.

Fix label colors for border and text were mixed up.
@lukasoppermann lukasoppermann requested review from a team and colebemis October 27, 2022 13:18
@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2022

🦋 Changeset detected

Latest commit: be6692a

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 Oct 27, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 78.13 KB (-0.01% 🔽)
dist/browser.umd.js 78.78 KB (-0.01% 🔽)

@lukasoppermann lukasoppermann temporarily deployed to github-pages October 27, 2022 13:25 Inactive
@josepmartins
Copy link
Contributor

This leads to an a11y issue were text colors don't pass.

Just curious about this, aren't the fg and emphasis values the same? what's the a11y issue?

Screenshot 2022-10-27 at 15 39 33

Copy link
Contributor

@josepmartins josepmartins left a comment

Choose a reason for hiding this comment

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

LGTM! just left a non blocking comment/question

@lukasoppermann
Copy link
Contributor Author

This leads to an a11y issue were text colors don't pass.

Just curious about this, aren't the fg and emphasis values the same? what's the a11y issue?

Screenshot 2022-10-27 at 15 39 33

Not in dark mode:

  done: {
    fg: get('scale.purple.4'),
    emphasis: get('scale.purple.5'),
    muted: alpha(get('scale.purple.4'), 0.4),
    subtle: alpha(get('scale.purple.4'), 0.15)
  },
  sponsors: {
    fg: get('scale.pink.4'),
    emphasis: get('scale.pink.5'),

@josepmartins
Copy link
Contributor

Not in dark mode:

Gotcha, thank you! 👍

@lukasoppermann
Copy link
Contributor Author

@colebemis or @rezrah can you help me with the failing changeset? What do I need to do?

@broccolinisoup
Copy link
Member

@colebemis or @rezrah can you help me with the failing changeset? What do I need to do?

Hi @lukasoppermann 👋🏼 It is complaining because there is no changeset file for this change. You can read about changesets here but for a brief intro, we use them for release tracking purposes. Changeset files include info about what we need to release and what level of versioning i.e. patch/minor/major (ref: semver )) I believe this PR is a patch. And you can run npx changeset on your branch and answer the questions that pop put and it should generate a changeset file and please commit that changeset file after. That should make the error go away and have a proper changeset for the release tracking. Let us know if you still have issues 🙂

@lukasoppermann lukasoppermann temporarily deployed to github-pages October 28, 2022 06:56 Inactive
@lukasoppermann lukasoppermann temporarily deployed to github-pages October 28, 2022 07:12 Inactive
@lukasoppermann lukasoppermann merged commit 6a30812 into main Oct 28, 2022
@lukasoppermann lukasoppermann deleted the lukasoppermann-fix-label-colors branch October 28, 2022 07:18
@primer-css primer-css mentioned this pull request Oct 28, 2022
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.

3 participants