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.

Sign up for GitHub

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

Jump to bottom

Development no longer restricted to dark theme only #365

Merged
merged 2 commits into from
Aug 28, 2018
Merged

Development no longer restricted to dark theme only #365

merged 2 commits into from
Aug 28, 2018

Conversation

petemill
Copy link
Member

@petemill petemill commented Aug 28, 2018 •
edited
Loading

Theme user preference can apply to any development build, not just official build, so that we can actually develop with the different themes and use the setting in development builds.

Also adds cli flag: ui-mode={light,dark}
This overrides the light / dark / default setting to aid developer productivity when testing UI changes on both modes (developer doesn't have to open settings page and make a change in order to switch between the two options).

Renames 'DevChannel' theme to DarkUi and 'ReleaseChannel' theme to LightUi.
Fix brave/brave-browser#863

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

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

@petemill petemill added the UI label Aug 28, 2018
@petemill petemill self-assigned this Aug 28, 2018
@petemill petemill requested review from bridiver and simonhong August 28, 2018 16:59
@petemill
Development no longer restricted to dark theme only
77031db 
Theme user preference can apply to any build.
Renames 'DevChannel' theme to DarkUi and 'ReleaseChannel' theme to LightUi.
Fix brave/brave-browser#863
@petemill petemill force-pushed the fix/863-development-theme branch from f362958 to 77031db Compare August 28, 2018 17:08
@petemill
Copy link
Member Author

Updated to add cli flag: ui-mode={light,dark}

@petemill petemill mentioned this pull request Aug 28, 2018
Merged
16 tasks
@simonhong
Copy link
Member

How about control development theme mode also by using setting options instead of cli?
With small change in #361, it can be possible.

@petemill
Copy link
Member Author

@simonhong that is exactly the intention of this PR - it removes the #if defined(OFFICIAL_BUILD) restriction so that the setting works in development too. Do you not think it achieves that?

The CLI flag is an additional tool to aid productivity when developing UI features so that a developer can easily switch between light and dark without having to open settings page

@simonhong
Copy link
Member

Ah, I just concerned the adding of cli.
I thought settings option also can do same job of the cli.
If it is useful, I don't mind adding it.

simonhong
simonhong previously approved these changes Aug 28, 2018
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

LGTM

common/brave_switches.cc Outdated
@@ -23,4 +23,6 @@ const char kDisablePDFJSExtension[] = "disable-pdfjs-extension";
// Allows disabling the Tor client updater extension.
const char kDisableTorClientUpdaterExtension[] = "disable-tor-client-updater-extension";

const char kUiMode[] = "ui-mode";
Copy link
Member

Choose a reason for hiding this comment

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

nit: how about adding possible option values in the comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

good spot @simonhong - I added a comment now. Please could you re-review as it was automatically discarded.

@petemill
Copy link
Member Author

Thanks @simonhong - I updated the PR description to explain this point

@petemill
New cli flag for switching built-in UI theme
6c69116 
--ui-mode=light
--ui-mode=dark

Overrides preference or channel default
@petemill petemill force-pushed the fix/863-development-theme branch from 4b57a72 to 6c69116 Compare August 28, 2018 22:51
simonhong
simonhong approved these changes Aug 28, 2018
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

LGTM

@petemill petemill merged commit baecd32 into brave:master Aug 28, 2018
@petemill petemill deleted the fix/863-development-theme branch August 28, 2018 23:01
fmarier pushed a commit that referenced this pull request Oct 29, 2019
@bbondy
Merge pull request #365 from brave/grd-strings
e366b5d 
Provide a script to do GRD string upgrades for Chromium rebasing
petemill pushed a commit that referenced this pull request Jul 27, 2020
@bbondy
Merge pull request #365 from brave/grd-strings
1448a6f 
Provide a script to do GRD string upgrades for Chromium rebasing
petemill pushed a commit that referenced this pull request Jul 28, 2020
@bbondy
Merge pull request #365 from brave/grd-strings
8aa093c 
Provide a script to do GRD string upgrades for Chromium rebasing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonhong simonhong simonhong approved these changes

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@petemill petemill

Labels
UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Light/Dark should be selectable in development
2 participants
@petemill @simonhong