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

Adjust sidebar colors #12578

Merged
merged 1 commit into from
Mar 16, 2022
Merged

Adjust sidebar colors #12578

merged 1 commit into from
Mar 16, 2022

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Mar 11, 2022

fix brave/brave-browser#21590

So far, sidebar can't aligned with custom theme
because sidebar only knows brave's dark/light theme color.

With below changes, sidebar colors are aligned well with custom theme also.

Used toolbar color as a sidebar background color.
Used toolbar button's hover color for sidebar item/arrow button hover color.
Picked proper color between light/dark color for builtin item icon color based on toolbar color.
Used toolbar area separator color(separator between toolbar and web contents) for sidebar border.

image image image image

image
image
image
image

Resolves

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • 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, npm run lint, 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:

  1. Launch brave
  2. Load brave://settings/manageProfile and try any theme color
  3. Check sidebar colors are aligned with new them like above screenshot

@simonhong simonhong self-assigned this Mar 11, 2022
@simonhong simonhong force-pushed the sidebar_custom_theme_aware branch from 20b618b to 641a976 Compare March 14, 2022 03:23
@simonhong simonhong changed the title WIP: Adjust sidebar colors Adjust sidebar colors Mar 14, 2022
@simonhong simonhong marked this pull request as ready for review March 14, 2022 05:26
@github-actions github-actions bot added rebase and removed rebase labels Mar 14, 2022
@simonhong simonhong force-pushed the sidebar_custom_theme_aware branch from 641a976 to 6dfe04f Compare March 14, 2022 05:33
@simonhong simonhong requested review from mkarolin and petemill March 14, 2022 05:38
@simonhong
Copy link
Member Author

@bradleyrichter PTAL this color changes. I can't add you in the reviewer list.

@simonhong simonhong force-pushed the sidebar_custom_theme_aware branch from 6dfe04f to 165d0e3 Compare March 15, 2022 00:41
@simonhong simonhong added this to the 1.38.x - Nightly milestone Mar 16, 2022
@simonhong simonhong requested a review from nullhook March 16, 2022 04:05
@nullhook
Copy link
Contributor

i see conflicts

fix brave/brave-browser#21590

So far, sidebar can't aligned with custom theme
because sidebar only knows brave's dark/light theme color.

With below changes, sidebar colors are aligned well with custom theme also.

Used toolbar color as a sidebar background color.
Used toolbar button's hover color for sidebar item/arrow button hover color.
Picked proper color between light/dark color for builtin item icon color based on toolbar color.
Used toolbar area separator color(separator between toolbar and web contents) for sidebar border.
@simonhong simonhong force-pushed the sidebar_custom_theme_aware branch from 165d0e3 to 92c133d Compare March 16, 2022 05:06
@simonhong
Copy link
Member Author

i see conflicts

Fixed!

Copy link
Contributor

@nullhook nullhook 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 3d4de0b into master Mar 16, 2022
@simonhong simonhong deleted the sidebar_custom_theme_aware branch March 16, 2022 22:13
@stephendonner
Copy link
Contributor

stephendonner commented Mar 17, 2022

Verified PASSED using

Brave 1.38.53 Chromium: 99.0.4844.74 (Official Build) nightly (64-bit)
Revision fee9a47e86e981802390cb0d41c5ed7ea93c4f6f-refs/branch-heads/4844@{#1060}
OS Windows 10 Version 21H2 (Build 19044.1586)

Confirmed creating a new, colored profile in both light and dark modes has a matching Sidebar
Confirmed favicons are appropriate for light and dark modes

example example example example example
21590-1 21590-2 21590-3 21590-4 21590-5

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.

Sidebar color does not adhere to a new profile theme color
3 participants