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

Prioritized updates #19655

Merged
merged 11 commits into from
Dec 5, 2024
Merged

Prioritized updates #19655

merged 11 commits into from
Dec 5, 2024

Conversation

niik
Copy link
Member

@niik niik commented Dec 3, 2024

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

A screenshot of GitHub Desktop using the dark theme showing a banner with the text This version of GitHub Desktop is missing important updates. Please restart GitHub Desktop now to install pending updates

A screenshot of GitHub Desktop using the light theme showing a banner with the text This version of GitHub Desktop is missing important updates. Please restart GitHub Desktop now to install pending updates

Release notes

Notes:

Copy link
Contributor

@tidy-dev tidy-dev left a 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.

Comment on lines 380 to 382
--banner-warning-background: #{darken($orange-900, 20%)};
--banner-warning-text-color: var(--text-color);
--banner-warning-link-color: #4285fb;
Copy link
Contributor

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. :)

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

Comment on lines 481 to 483
--banner-warning-background: #{$yellow-700};
--banner-warning-text-color: var(--text-color);
--banner-warning-link-color: #{$blue-700};
Copy link
Contributor

@tidy-dev tidy-dev Dec 4, 2024

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);
        }
    ...

Showing using same styles as from bidi character warning

@niik
Copy link
Member Author

niik commented Dec 5, 2024

I have left a comment as we have one accessibility contrast issue to resolve, but other than that looks great.

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

A screenshot of GitHub Desktop using the dark theme showing a banner with the text This version of GitHub Desktop is missing important updates. Please restart GitHub Desktop now to install pending updates
A screenshot of GitHub Desktop using the light theme showing a banner with the text This version of GitHub Desktop is missing important updates. Please restart GitHub Desktop now to install pending updates

Copy link
Contributor

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

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

Super close... :)

In light and dark theme we are not quite to the 3:1 for link to body text.

CleanShot 2024-12-05 at 09 00 22

CleanShot 2024-12-05 at 09 05 23

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.

CleanShot 2024-12-05 at 09 02 06

CleanShot 2024-12-05 at 09 05 47

@tidy-dev
Copy link
Contributor

tidy-dev commented Dec 5, 2024

Lol.. after I posted I didn't catch that the link to background failing now.

image

So, we need to figure that out... I wonder if bidi character is failing that.

@niik
Copy link
Member Author

niik commented Dec 5, 2024

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

image

image

Copy link
Contributor

@tidy-dev tidy-dev left a 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!

@niik niik enabled auto-merge December 5, 2024 14:27
@niik niik merged commit e02fb6d into development Dec 5, 2024
7 checks passed
@niik niik deleted the priority-updates branch December 5, 2024 14:34
@72158895omar1
Copy link

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

A screenshot of GitHub Desktop using the dark theme showing a banner with the text This version of GitHub Desktop is missing important updates. Please restart GitHub Desktop now to install pending updates

A screenshot of GitHub Desktop using the light theme showing a banner with the text This version of GitHub Desktop is missing important updates. Please restart GitHub Desktop now to install pending updates

Release notes Omar a Rodríguez botja

Notes:

72158895omat1

@F-fengzi
Copy link

That did not take too long to put into use! :D

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.

5 participants