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

Accessibility fixes for marketing buttons #716

Merged
merged 7 commits into from
Mar 13, 2019
Merged

Conversation

broccolini
Copy link
Member

@broccolini broccolini commented Mar 10, 2019

This pr is a follow-up to https://github.com/github/github/pull/109480 which fixes color contrast on buttons and text for our logged-in homepage. We need to fix this on the logged out homepage too and any other sites (such as help) that use these styles. So it makes sense to fix in Primer.

Can someone else take this on and get updated in dotcom asap? We're aiming to get as much as possible fixed by 3/11/19 (this Monday).

@broccolini broccolini marked this pull request as ready for review March 10, 2019 16:29
Copy link
Contributor

@shawnbot shawnbot 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, but let’s wait on a release branch to merge.

@broccolini
Copy link
Member Author

broccolini commented Mar 10, 2019

Looks good, but let’s wait on a release branch to merge.

Yep, apologies this is off master. If possible would be great to get a small release into dotcom quickly 😬. I definitely the site designers to take a look first though since it will impact other websites.

@shawnbot shawnbot changed the base branch from master to release-12.1.2 March 11, 2019 17:10
@shawnbot
Copy link
Contributor

Cool, no prob. I'm stubbing out the release now, so it should be ready by the time you get a review from @skullface and @gladwearefriends.

Copy link
Contributor

@skullface skullface left a comment

Choose a reason for hiding this comment

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

Originally $blue-450 was supposed to be a 42% mix (https://codepen.io/skullface/pen/f5f527ce4afcc91c125e2d192df94de9?editors=0100#0 — as opposed to 50% as implemented) to pass AAA large and AA normal ratio requirements.

I’m 👍 okay with removing 450 in favor of 500 as my use is pretty strictly on dotcom pages, but I shouldn’t be the only Site Design voice here. @gladwearefriends and @trosage’s work using 450 touches more unique designs and other GitHub sites like @broccolini mentioned.

Copy link
Contributor

@trosage trosage left a comment

Choose a reason for hiding this comment

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

I'd prefer keeping $blue-450 but redefining the variable to 42% as @skullface mentioned

@gladwearefriends
Copy link
Contributor

yes could we do 450 at 42%?

id prefer it to be brighter because we have the marketing buttons on dark blue backgrounds, and the darker 500 makes the buttons a little muddy compared to the brighter option.

450 (at 42%)
Screen Shot 2019-03-11 at 12 50 10 PM

500
Screen Shot 2019-03-11 at 12 49 17 PM

@broccolini
Copy link
Member Author

Thanks everyone for the deeper context, that's really helpful. Here's a few things to consider, but seems like it won't be too difficult to find accessible solution you're happy with.

  1. I understand wanting to adjust colors in context, however we need to be specific about their use. Can we restrict the alternative blue button color to buttons only? It looks too light and odd against other colors when used as a link, I think we should stick with our standard link blue. Here's an example on the new help website:

Screenshot 2019-03-11 16 57 26

  1. The variable name shouldn't look/act like a color system variable when it's not a color system variable. This could cause confusion elsewhere and lead people to thinking we have 50 steps for other numbers. Something like $blue-mktng could work? Let me know what you think re naming.

  2. Whatever we do with color, we need to make a serious effort to have colors pass contrast. The recent updates to the homepage degraded our accessibility standards. This has an impact to our customers and has wider implications outlined in this issue. Design Systems should have caught this when the changes were added, we'll keep a closer eye on that in future. It will greatly help if you can test site design work before shipping. I recommend using the Accessibility Insights Chrome plugin, in the dropdown you'll see an option called "Fast pass" which will highlight elements on the page as well as give you a report summary. If you have any questions let us know!

  3. Generally we want to make sure the color system works for common use cases. Color functions aren't bad but if we find we're performing a color function frequently on the same variable, that's an indicator that there's a problem. If you find that you encounter problems, we want to know about them.! Aka is $blue-400 too light, or is the background your using $blue-800 too dark etc. Please holla if you have feedback!


As a quick next step, would like to hear feedback on the color variable name and context of use.

@simurai simurai closed this Mar 12, 2019
@simurai simurai changed the base branch from release-12.1.2 to release-12.2.0 March 12, 2019 08:26
@simurai simurai reopened this Mar 12, 2019
@trosage
Copy link
Contributor

trosage commented Mar 12, 2019

@broccolini I think that's a fair compromise. But I think we should just name the color variable $blue-442. Just kidding 🤪$blue-mktg works for me.

@simurai simurai mentioned this pull request Mar 12, 2019
10 tasks
@broccolini
Copy link
Member Author

I've updated the variables in this pr, please test and check changes before shipping!

@shawnbot shawnbot self-requested a review March 12, 2019 21:54
@shawnbot shawnbot requested a review from trosage March 12, 2019 21:54
@shawnbot
Copy link
Contributor

This looks good to me (compared with the live site), but @trosage I'd love your eyes on it too if you've got some time. 🙇

Copy link
Contributor

@gladwearefriends gladwearefriends 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! thanks for working with us to catch this and help make things more accessible ✨

@skullface
Copy link
Contributor

LGTM too! Thank you, DS friends 💕

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.

6 participants