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

Uplift #1805 to 0.63 - OS appearance mode controls Brave appearance mode (light / dark theme) #1936

Merged
merged 1 commit into from
Mar 15, 2019

Conversation

petemill
Copy link
Member

@petemill petemill commented Mar 13, 2019

This is an uplift request for 0.63.x of #1805.
Reason for uplift request is that C63 has the 'Same as macOS' theme functionality launched in chrome. That is, the chrome light / dark mode will follow the macOS light / dark appearance mode. Without this PR, Brave will not automatically change, and has to be done manually from Settings.

Therefore this can be seen as a regression fix for 2 reasons:

  1. There is a mismatch between some Brave controls when choosing a theme that is different to the user's OS theme. I.e. the toolbar can be in a light theme whilst the info popups and menus would be in a dark theme.

Like this:
image

  1. Upstream supports changing light / dark mode based on OS but Brave breaks this functionality.

Fix brave/brave-browser#1189
Fix brave/brave-browser#1289

OS(MacOS/Windows) theme controls brave theme
@petemill petemill self-assigned this Mar 13, 2019
@petemill petemill requested a review from a team March 13, 2019 22:46
@simonhong simonhong added this to the 0.63.x - Dev milestone Mar 13, 2019
@petemill petemill changed the title Uplift #1805 - OS appearance mode controls Brave appearance mode (light / dark theme) Uplift #1805 to 0.63 - OS appearance mode controls Brave appearance mode (light / dark theme) Mar 14, 2019
Copy link
Member

@kjozwiak kjozwiak left a comment

Choose a reason for hiding this comment

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

Uplift to 0.63.x approved after deliberating with @srirambv & @bbondy. Please make sure that all associated issues are moved into the correct milestones.

@petemill before merging, can you double check the Jenkins failure and make sure there's no issues?

@simonhong
Copy link
Member

After merging this, we should uplift #1958 also.
This PR will cause compile error on buildbot.
I'll check two PR on beta/dev branch on my local first.

@kjozwiak
Copy link
Member

After merging this, we should uplift #1958 also.
This PR will cause compile error on buildbot.
I'll check two PR on beta/dev branch on my local first.

Thanks @simonhong, let me know if we end up needing #1958 as well.

@simonhong
Copy link
Member

@kjozwiak This PR was fine on my local but want to double check about the log from This commit cannot be built. Can you paste that log here? I think I need to request to access permission soon :)

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.

3 participants