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

Increase border contrast for Label--yellow #1126

Merged
merged 2 commits into from
Aug 3, 2020
Merged

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Jul 21, 2020

This makes the border of the Label--yellow a bit darker:

Before After
600 800
yellow-600 yellow-800

Reasoning

We got feedback that the yellow labels are harder to see: https://github.com/github/github/issues/149846 (internal)

Alternatives

We could also use yellow-700 for the border.

700

It would fail the AA color contrast ratio for graphical objects. But at the same time it would be more recognizable as "yellow".

@simurai simurai requested review from auareyou, a team and colinkeany and removed request for a team July 21, 2020 04:30
@vercel
Copy link

vercel bot commented Jul 21, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-css/5xg7dq35j
✅ Preview: https://primer-css-git-label-yellow.primer.vercel.app

Copy link
Contributor

@colinkeany colinkeany left a comment

Choose a reason for hiding this comment

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

I would suggest, for the sake of passing AA, that we use -800. I think it's more important that the word "Pending" is passing. The board color is mostly a visual treatment that supports the status.

@simurai
Copy link
Contributor Author

simurai commented Jul 22, 2020

@colinkeany I would suggest, for the sake of passing AA, that we use -800. I think it's more important that the word "Pending" is passing.

To clarify.. this PR only changes the border color. The text color is already using the darkest yellow-900 and that passes AA. ✅

@simurai simurai requested a review from colinkeany July 22, 2020 03:18
@colinkeany
Copy link
Contributor

@colinkeany I would suggest, for the sake of passing AA, that we use -800. I think it's more important that the word "Pending" is passing.

To clarify.. this PR only changes the border color. The text color is already using the darkest yellow-900 and that passes AA. ✅

Yes sorry if that wasn't clear! I was speaking about the border color.

@simurai
Copy link
Contributor Author

simurai commented Jul 22, 2020

Sorry.. ok, I see. Your point is that the border is just as important as the text and should pass AA too. 👍

@auareyou any objections? Here a border color of yellow-800 compared to the other labels:

image

@auareyou
Copy link
Contributor

Sorry.. ok, I see. Your point is that the border is just as important as the text and should pass AA too. 👍

@auareyou any objections? Here a border color of yellow-800 compared to the other labels:

image

I think that for the sake of uniformity yellow-800 makes more sense and it seems more consistent with the rest.

@vercel vercel bot temporarily deployed to Preview July 23, 2020 00:14 Inactive
@simurai
Copy link
Contributor Author

simurai commented Jul 23, 2020

yellow-800 makes more sense

👍 Ok, the border is now yellow-800.

Copy link
Contributor

@auareyou auareyou left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🎨

@simurai simurai changed the base branch from master to release-15.1.0 August 3, 2020 00:27
@simurai simurai merged commit ca25868 into release-15.1.0 Aug 3, 2020
@simurai simurai deleted the label-yellow branch August 3, 2020 00:27
@simurai simurai mentioned this pull request Aug 3, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants