-
Notifications
You must be signed in to change notification settings - Fork 868
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
[Nala]: Remove redundant overrides, to improve theming support. #25337
Conversation
8eec38f
to
4d66a6b
Compare
@simonhong mind taking a look at the color changes? I'll go over this with @aguscruiz tomorrow |
c077eb7
to
1d9c5f9
Compare
For the Sidebar issue, having an outline, this is a place where we differ from chromium. I need to see if we want to add an outline in the panel content's frame, to separate them from the chrome background (which is now gonna be the same color). But let me get back to you on that. In the meantime, we should remove those dividers. They won't work with our window padding |
1d9c5f9
to
f9b9f3b
Compare
f9b9f3b
to
320374f
Compare
320374f
to
4674ee5
Compare
@aguscruiz we have a separator between vertical tabs and the content area still - seems like we have this in release too but its a bit more subtle: Fixed separator @simonhong @aguscruiz I think I've improved the dark mode with a pretty minimal change, which hopefully @rebron will like: Hover states are pretty subtle, unfortunately (see last screenshot) The neutral colors have a slightly redder tint than our existing ones, but I can try and use our Nala ones, if you like @aguscruiz? |
4674ee5
to
05acac5
Compare
@simonhong I'm going to fix those in a separate PR, just checked, it's not in 1.70.x but it is in current Nightly |
Oops, thanks for checking. I thought it happens with this PR. |
8aef13e
to
812afd4
Compare
A Storybook has been deployed to preview UI for the latest push |
A Storybook has been deployed to preview UI for the latest push |
A Storybook has been deployed to preview UI for the latest push |
627b6b1
to
fb089ae
Compare
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.
++
fb089ae
to
40a5020
Compare
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.
lgtm with few things to cleanup. nice work!
e0144c5
to
64fec80
Compare
Thanks for all your help @simonhong & @goodov 😄 @aguscruiz could you take another look at the PR and see if the SplitView changes look good? |
Just checked on Windows, all colors are looking great, seal of approval on my end to test this out on nightly and see how it feels :D |
64fec80
to
c98281e
Compare
- Add tests for overrides - Improve approach for SysColorMixer override
c98281e
to
0b55d49
Compare
[puLL-Merge] - brave/brave-core@25337 Here's a description of the main changes in this PR: DescriptionThis PR adjusts the color system in Brave to align more closely with Material Design principles while maintaining some Brave-specific customizations. It updates the theme color picker, modifies various UI element colors, and refines how colors are applied in different theme modes (light, dark, private). ChangesChanges
Possible Issues
Security HotspotsNo significant security issues are apparent in this change. |
Resolves brave/brave-browser#40689
Notes:
I'm not sure whether to merge this as three separate PRs
or as one. For now, the three commits are in one PR so @aguscruiz can more easily see how all the changes are going to work together.
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: