Conversation
tidy-dev
left a comment
There was a problem hiding this comment.
Looking good!
I have left a comment as we have one accessibility contrast issue to resolve, but other than that looks great.
app/styles/themes/_dark.scss
Outdated
| --banner-warning-background: #{darken($orange-900, 20%)}; | ||
| --banner-warning-text-color: var(--text-color); | ||
| --banner-warning-link-color: #4285fb; |
There was a problem hiding this comment.
Good contrast for dark mode. :)
app/styles/_variables.scss
Outdated
| --banner-warning-background: #{$yellow-700}; | ||
| --banner-warning-text-color: var(--text-color); | ||
| --banner-warning-link-color: #{$blue-700}; |
There was a problem hiding this comment.
For accessibility, we need:
link -> text = 3:1
link/text -> background: 4.5:1
Unfortunately, this is giving link:text of 1.5:1 for light mode.
Here is a link checker tool that has inputs for the link, the text, and the background to make verification easier.
https://webaim.org/resources/linkcontrastchecker/
Feel free to fix as you see fit. When I was testing locally, I tried the hidden bidi characters styles (which have been modified recently for accessibility.) These colors were inspired by warning colors of https://primer.style/components/banner. ... which one could argue that maybe we could use there "Critical" banner styles which would be a redish color kind of like dark mode. https://primer.style/foundations/primitives/color has a --bgColor-danger-muted (for background) and --bgColor-danger-emphasis (for icon) -- but would need to be verified for accessibility. (Notes, the icon needs to have 3:1 to background)
/** Banner */
--banner-warning-background: #{$yellow-100}; // copied from --file-warning-background-color
--banner-warning-text-color: var(--text-color);
--banner-warning-link-color: var(--link-button-color);
/_update-available.css
&.priority {
.octicon {
color: var(--file-warning-color);
}
...
Thanks a bunch. I aligned the dark banner with the file warning in the dark theme and (believe) I've covered all the contrast issues
|
There was a problem hiding this comment.
Super close... :)
In light and dark theme we are not quite to the 3:1 for link to body text.
I found if we used the original link button blue it works on both dark/light themes, so I think we can just not apply any special css to the links in this banner and we will be good.
|
Lol.. after I posted I didn't catch that the link to background failing now.
So, we need to figure that out... I wonder if bidi character is failing that. |
|
Alright, I somehow missed the contrast between the text and the link and only cared about link to background. Here's new results for the light theme and dark theme
|
tidy-dev
left a comment
There was a problem hiding this comment.
💖 Thank you for those fixes!
72158895omat1 |
|
That did not take too long to put into use! :D |
xref https://github.com/github/desktop/issues/894
Description
From time to time we ship new releases containing fixes for high severity security vulnerabilities. Most frequently this has been a vulnerability in a third-party dependency (Git, Git LFS etc) but it could also be a vulnerability within Desktop itself. Over Desktop's lifetime we've had a few releases where we've wanted to get the word out to users that they should update as soon as possible to stay safe.
With this change we'll be able to communicate to Desktop clients that they should prioritize updating to the latest version ASAP. When we do so we'll make remove the ability to dismiss the update banner, as well as change the styling and text to convey the urgency of updating.
Screenshots
Release notes
Notes: