Add brave theme option to settings page #361

simonhong · 2018-08-26T23:31:34Z

Close brave/brave-browser#789

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:

Open settings page and check theme is changed when brave color option changes.

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

simonhong · 2018-08-26T23:41:42Z

@bbondy @bridiver I marked as WIP for avoiding patching files of html/js files.
Can you advice for this?

bridiver · 2018-08-27T18:03:18Z

patches/chrome-browser-resources-settings-appearance_page-appearance_browser_proxy.js.patch

+index c75464dc66ecedd8dbfac47b0dbdb4b6c68fd969..fd62faf174909c5c385277c08bb782bd4127d442 100644
+--- a/chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js
+++ b/chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js
+@@ -45,6 +45,17 @@ cr.define('settings', function() {


I don't understand why we're using this at all if we have our own webui?

brave_appearance_browser_proxy.js is introduced.

bridiver · 2018-08-27T18:05:59Z

patches/chrome-browser-resources-settings-appearance_page-appearance_page.html.patch

+             </template>
+           </select>
+         </div>
+<if expr="not _google_chrome">


same with this. Maybe it makes more sense to keep this separate from chrome webstore theme prefs? If we want to keep it under the same heading, seems like we should be able to use the templating engine to insert an html snippet here without patching the whole thing in?

something like {{brave_theme_settings}} that contains all of this? Not sure if that is possible or not

settings-brave-apperance-page module is only added.

bridiver · 2018-08-27T18:06:57Z

patches/chrome-browser-resources-settings-appearance_page-appearance_page.js.patch

+       ],
+     },
+
+    // <if expr="not _google_chrome">


I think we can include another js file here to do this. Talk to @yrliou because she did something similar

example here 45c8a93

brave_appearance_page.js is introduced.

simonhong · 2018-08-28T09:16:12Z

One more thing is remained to remove WIP.
Below entries should be added to settings_resources.grd.

<structure name="IDR_SETTINGS_BRAVE_APPEARANCE_BROWSER_PROXY_HTML" file="brave_appearance_page/brave_appearance_browser_proxy.html" type="chrome_html" />
<structure name="IDR_SETTINGS_BRAVE_APPEARANCE_BROWSER_PROXY_JS" file="brave_appearance_page/brave_appearance_browser_proxy.js" type="chrome_html" preprocess="true" />
<structure name="IDR_SETTINGS_BRAVE_APPEARANCE_PAGE_HTML" file="brave_appearance_page/brave_appearance_page.html" type="chrome_html" preprocess="true" allowexternalscript="true" />
<structure name="IDR_SETTINGS_BRAVE_APPEARANCE_PAGE_JS" file="brave_appearance_page/brave_appearance_page.js" type="chrome_html" preprocess="true" />

@bbondy , can you advice howto add above items to settings_resources.grd?
I saw the comment that this file is automatically generated.

yrliou · 2018-08-28T11:42:39Z

@simonhong See script/chromium-rebase-l10n.py for adding entries, you can see brave_page_visibility.js as an example.

petemill · 2018-08-28T16:18:56Z

This option is still present when the user has installed a theme extension? The intention is for this toggle to still be present as there are some UI elements that are non-extension-themeable so we will still want the option for those items.

petemill · 2018-08-28T16:33:41Z

patches/chrome-browser-resources-settings-appearance_page-appearance_page.html.patch

+             </template>
+           </select>
+         </div>
+        <settings-brave-appearance-page></settings-brave-appearance-page>


Please can we put this at the top of the appearance section? So that it is next to the extension theme item.

simonhong · 2018-08-28T22:21:05Z

@petemill Yes, our theme option is always effective regardless of user theme extension is installed or not.
When user theme extension is installed, our theme properties only overrides colors that user theme doesn't override because we only overrided ThemeService::GetDefaultColor().

petemill · 2018-08-29T00:00:22Z

app/brave_generated_resources.grd

@@ -138,6 +138,10 @@ By installing this extension, you are agreeing to the Google Widevine Terms of U
      <message name="IDS_BLOCKED_AUTOPLAY_NO_ACTION" desc="Radio button choice to continue blocking a site from autoplay media, displayed in bubble when a page tries to autoplay media.">
        Continue blocking autoplay
      </message>
+      <!-- Appearance -->
+      <message name="IDS_SETTINGS_APPEARANCE_SETTINGS_BRAVE_THEMES" desc="The label for brave theme change setting options">
+        Brave Themes


I had a talk to the design team, please change this string value to Brave colors

simonhong · 2018-08-29T02:13:34Z

Removed wip label. I think this is ready to review again!

simonhong · 2018-08-30T23:35:25Z

Kindly ping

With this module, we can simply our apperance options to settings page.

With this, four web resources are added for brave theme options. * brave_appearance_browser_proxy.html * brave_appearance_browser_proxy.js * brave_appearance_page.html * brave_appearance_page.js

bbondy

lgtm

simonhong self-assigned this Aug 26, 2018

simonhong requested review from bridiver and bbondy August 26, 2018 23:31

simonhong changed the title ~~Add brave theme option to settings page~~ [WIP] Add brave theme option to settings page Aug 26, 2018

simonhong added the pr/work-in-progress label Aug 26, 2018

simonhong force-pushed the brave_theme_option branch from ad6f91c to 3828ccc Compare August 26, 2018 23:39

simonhong force-pushed the brave_theme_option branch 2 times, most recently from 6e122a8 to d0d1e1c Compare August 27, 2018 03:12

petemill mentioned this pull request Aug 27, 2018

Ability to switch between Dark / Light mode brave/brave-browser#789

Closed

2 tasks

bridiver reviewed Aug 27, 2018

View reviewed changes

simonhong mentioned this pull request Aug 28, 2018

Copy brave_appearance_page module brave/brave-browser#860

Merged

16 tasks

petemill reviewed Aug 28, 2018

View reviewed changes

simonhong mentioned this pull request Aug 28, 2018

Development no longer restricted to dark theme only #365

Merged

11 tasks

petemill reviewed Aug 29, 2018

View reviewed changes

simonhong force-pushed the brave_theme_option branch 2 times, most recently from 90a1f33 to c2fd3fc Compare August 29, 2018 02:11

simonhong changed the title ~~[WIP] Add brave theme option to settings page~~ Add brave theme option to settings page Aug 29, 2018

simonhong removed the pr/work-in-progress label Aug 29, 2018

simonhong requested a review from yrliou August 29, 2018 02:15

simonhong force-pushed the brave_theme_option branch from c2fd3fc to 7caab82 Compare August 29, 2018 02:24

Add brave theme option to settings page

5bc52e7

simonhong added 2 commits September 1, 2018 08:03

Introduce settings-brave-appearance-page module

59c7634

With this module, we can simply our apperance options to settings page.

Update chromium-rebase-l10n.py to handle more resources

1bd7ce5

With this, four web resources are added for brave theme options. * brave_appearance_browser_proxy.html * brave_appearance_browser_proxy.js * brave_appearance_page.html * brave_appearance_page.js

simonhong force-pushed the brave_theme_option branch from 7caab82 to 1bd7ce5 Compare September 3, 2018 03:03

bbondy approved these changes Sep 3, 2018

View reviewed changes

bbondy merged commit 50fd710 into master Sep 3, 2018

simonhong deleted the brave_theme_option branch September 4, 2018 03:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add brave theme option to settings page #361

Add brave theme option to settings page #361

simonhong commented Aug 26, 2018 •

edited

Loading

simonhong commented Aug 26, 2018

bridiver Aug 27, 2018

simonhong Aug 28, 2018

bridiver Aug 27, 2018

bridiver Aug 27, 2018

simonhong Aug 28, 2018

bridiver Aug 27, 2018

bridiver Aug 27, 2018

simonhong Aug 28, 2018

simonhong commented Aug 28, 2018

yrliou commented Aug 28, 2018

petemill commented Aug 28, 2018

petemill Aug 28, 2018

simonhong Aug 29, 2018

simonhong commented Aug 28, 2018 •

edited

Loading

petemill Aug 29, 2018

simonhong Aug 29, 2018

simonhong commented Aug 29, 2018

simonhong commented Aug 30, 2018

bbondy left a comment

Add brave theme option to settings page #361

Add brave theme option to settings page #361

Conversation

simonhong commented Aug 26, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

simonhong commented Aug 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Aug 28, 2018

yrliou commented Aug 28, 2018

petemill commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Aug 29, 2018

simonhong commented Aug 30, 2018

bbondy left a comment

Choose a reason for hiding this comment

simonhong commented Aug 26, 2018 •

edited

Loading

simonhong commented Aug 28, 2018 •

edited

Loading