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

Update update alert color from app menu for light theme #24086

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jun 10, 2024

fix brave/brave-browser#38899

Brave light/dark themes:
Screenshot 2024-06-12 at 12 46 34 PM
Screenshot 2024-06-12 at 12 46 42 PM
Screenshot 2024-06-12 at 12 48 39 PM
Screenshot 2024-06-12 at 12 48 48 PM
Screenshot 2024-06-12 at 12 51 19 PM
Screenshot 2024-06-12 at 12 51 26 PM

Resolves

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

See the linked issue

@simonhong simonhong self-assigned this Jun 10, 2024
@simonhong simonhong requested a review from aguscruiz June 10, 2024 04:25
@simonhong
Copy link
Member Author

@aguscruiz As I describe above, I set raw color(SkColorSetRGB(0xE2, 0xA5, 0x00)) w/o contrast adjusting on light theme.
This looks fine or need to adjust?

@aguscruiz
Copy link
Collaborator

Sorry, I don't think I follow completely.

So the problem is that if you adjust the color for contrast, they change to gray in light mode? Shouldn't it change to a darker shade of orange though?

I think we're missing contrast this way

Dark mode looks fine

@simonhong simonhong force-pushed the update_app_menu_update_alert_color branch from d481059 to e8ae475 Compare June 11, 2024 02:16
@simonhong
Copy link
Member Author

Sorry, I don't think I follow completely.

So the problem is that if you adjust the color for contrast, they change to gray in light mode? Shouldn't it change to a darker shade of orange though?

I think we're missing contrast this way

Dark mode looks fine

Updated and attached some screenshots. WDYT?

@aguscruiz
Copy link
Collaborator

It doesn't techincally pass the contrast checks, but I think it's way more readable now.
I was gonna suggest to make the contrast on light mode more noticeable, but maybe we can wait until the material color updates. I would like to use Nala colors for this

Current, with contrast of 2.72:1
image

Ideal contrast of 4.5:1:
image

@aguscruiz
Copy link
Collaborator

aguscruiz commented Jun 11, 2024

I have the design using Nala colors, but I don't know how it'll look if you use the current token set in Nala since it's not updated to material colors yet.

https://www.figma.com/design/H11ZOl6JMYbCTW4ZJXqR5V/%F0%9F%A6%81-Browser?node-id=6904-11703&t=Rs2fzFw8U7kMgCVc-1

image

@simonhong simonhong force-pushed the update_app_menu_update_alert_color branch from e8ae475 to 6a99804 Compare June 12, 2024 03:53
@simonhong simonhong marked this pull request as ready for review June 12, 2024 03:53
@simonhong simonhong requested a review from sangwoo108 June 12, 2024 03:53
Copy link
Contributor

@sangwoo108 sangwoo108 left a comment

Choose a reason for hiding this comment

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

LGTM

@simonhong simonhong merged commit ced879c into master Jun 12, 2024
19 checks passed
@simonhong simonhong deleted the update_app_menu_update_alert_color branch June 12, 2024 12:52
@github-actions github-actions bot added this to the 1.69.x - Nightly milestone Jun 12, 2024
brave-builds added a commit that referenced this pull request Jun 12, 2024
brave-builds added a commit that referenced this pull request Jun 12, 2024
kjozwiak pushed a commit that referenced this pull request Jun 13, 2024
kjozwiak pushed a commit that referenced this pull request Jun 13, 2024
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.

[CR 126] hamburger "Update" prompt is missing a color when using light theme
3 participants