-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Prioritized updates #19655
Prioritized updates #19655
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good contrast for dark mode. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you yes couldnt agree more
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
}
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💖 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: