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

Allow theme change during Welcome onboarding #2909

Merged
merged 6 commits into from
Jul 26, 2019
Merged

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Jul 12, 2019

Fixes brave/brave-browser#4992
Closes brave/brave-browser#4657
Closes brave/brave-browser#3911

Brave-ui (Dependency): brave/brave-ui#487

Quick view:
welcome-theme

Test Plan:

  • welcome page displays in dark mode/light mode/system depending on user settings
  • the theme step of the welcome page has a confirm button that is disabled unless valid option is selected in the dropdown
  • selecting a valid option in the dropdown will change browser theme accordingly
  • selecting confirm will go navigate to the next step of on-boarding
  • On operating systems that don't support dark mode, the option (System theme (default)) will not show up in the dropdown

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@bsclifton bsclifton added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Jul 12, 2019
@bsclifton
Copy link
Member Author

bsclifton commented Jul 12, 2019

On any Web UI page, you can demo the get (was already present) and the set (which was added by my commit) by opening the console and trying the following:

chrome.braveTheme.getBraveThemeList(function (types) { console.log(types)})
> undefined
VM219:1 [{"name":"Same as macOS","value":0},{"name":"Dark","value":1},{"name":"Light","value":2}]

chrome.braveTheme.setBraveThemeType('Light')
> undefined // theme will change to light mode

chrome.braveTheme.setBraveThemeType('Dark')
> undefined // theme will change to dark mode

chrome.braveTheme.setBraveThemeType('LOLOL')
> undefined // theme will change to "default"

@imptrx imptrx closed this Jul 15, 2019
@imptrx imptrx force-pushed the bsc-onboard-theme branch from 521dfd1 to d09f054 Compare July 15, 2019 20:31
@imptrx imptrx reopened this Jul 15, 2019
@imptrx imptrx force-pushed the bsc-onboard-theme branch 3 times, most recently from 60c8054 to 4c98bf3 Compare July 23, 2019 00:08
@imptrx imptrx changed the title WIP: Allow theme change during Welcome onboarding Allow theme change during Welcome onboarding Jul 23, 2019
@imptrx imptrx mentioned this pull request Jul 23, 2019
@imptrx imptrx added dependencies Pull requests that update a dependency file dependency/brave-ui feature/welcome page labels Jul 23, 2019
@imptrx imptrx added this to the 0.69.x - Nightly milestone Jul 23, 2019
@imptrx imptrx force-pushed the bsc-onboard-theme branch from 3dad384 to f3fba26 Compare July 23, 2019 19:02
@imptrx imptrx marked this pull request as ready for review July 23, 2019 19:29
@imptrx imptrx requested a review from cezaraugusto July 23, 2019 19:30
@imptrx imptrx force-pushed the bsc-onboard-theme branch 2 times, most recently from eb24a52 to cf1a226 Compare July 23, 2019 21:57
cezaraugusto
cezaraugusto previously approved these changes Jul 23, 2019
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

++

simonhong
simonhong previously approved these changes Jul 23, 2019
@imptrx imptrx dismissed stale reviews from simonhong and cezaraugusto via 12d3066 July 23, 2019 23:23
@imptrx imptrx force-pushed the bsc-onboard-theme branch from cf1a226 to 12d3066 Compare July 23, 2019 23:23
cezaraugusto
cezaraugusto previously approved these changes Jul 23, 2019
@imptrx imptrx force-pushed the bsc-onboard-theme branch 3 times, most recently from d80b096 to a89163b Compare July 25, 2019 20:33
cezaraugusto
cezaraugusto previously approved these changes Jul 25, 2019
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

Screen Shot 2019-07-25 at 18 01 17

@imptrx imptrx modified the milestones: 0.69.x - Dev, 0.70.x - Nightly Jul 26, 2019
@imptrx imptrx closed this Jul 26, 2019
@imptrx imptrx force-pushed the bsc-onboard-theme branch from a89163b to f8509ef Compare July 26, 2019 17:57
@imptrx
Copy link
Contributor

imptrx commented Jul 26, 2019

Merging in as:

  • CI passed all tests but was failing/unstable due to dependency audit issue unrelated to this PR from rebase on July 25
  • CI is failing from rebase on July 26 due to a version mismatch between brave-browser and brave-core which is also unrelated to the changes here

@imptrx imptrx merged commit 236c089 into master Jul 26, 2019
@imptrx imptrx deleted the bsc-onboard-theme branch July 26, 2019 22:34
@petemill
Copy link
Member

Does this drop down still show System theme on OS which does not have system theme (Win10 before 2019, Win8/7, macOS before 2018, Linux)? The purpose of the API which retrieved the list of themes is to make the decision about which are supported on the current system. We should probably use that same code path. Plus, if we’re intending to change the language from Same as xyzOS to System Theme, then we should probably be consistent about that in both Welcome and Settings UI.

@imptrx
Copy link
Contributor

imptrx commented Jul 27, 2019

@petemill This drop down won't show the Same as xyzOS/System theme if system doesn't support it - I extrapolated that detail from the array length from the getBraveThemeList API

cc: @rossmoody @rebron for thought on parity between the dropdowns for on-boarding and settings page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS dependencies Pull requests that update a dependency file dependency/brave-ui feature/welcome page
Projects
None yet
5 participants