Resolved feedback

brave · Dec 15, 2022 · 09cab4f · 09cab4f
1 parent 95657c7
commit 09cab4f
Show file tree

Hide file tree

Showing 6 changed files with 51 additions and 42 deletions.
diff --git a/browser/ui/webui/brave_welcome_ui.cc b/browser/ui/webui/brave_welcome_ui.cc
@@ -11,7 +11,6 @@
 
 #include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
-#include "brave/browser/themes/brave_dark_mode_utils.h"
 #include "brave/browser/ui/webui/brave_webui_source.h"
 #include "brave/browser/ui/webui/settings/brave_import_data_handler.h"
 #include "brave/browser/ui/webui/settings/brave_search_engines_handler.h"
@@ -76,7 +75,13 @@ constexpr webui::LocalizedString kLocalizedStrings[] = {
     {"braveWelcomeChangeSettingsNote", IDS_BRAVE_WELCOME_CHANGE_SETTINGS_NOTE},
     {"braveWelcomePrivacyPolicyNote", IDS_BRAVE_WELCOME_PRIVACY_POLICY_NOTE},
     {"braveWelcomeSelectThemeLabel", IDS_BRAVE_WELCOME_SELECT_THEME_LABEL},
-    {"braveWelcomeSelectThemeNote", IDS_BRAVE_WELCOME_SELECT_THEME_NOTE}};
+    {"braveWelcomeSelectThemeNote", IDS_BRAVE_WELCOME_SELECT_THEME_NOTE},
+    {"braveWelcomeSelectThemeSystemLabel",
+     IDS_BRAVE_WELCOME_SELECT_THEME_SYSTEM_LABEL},
+    {"braveWelcomeSelectThemeLightLabel",
+     IDS_BRAVE_WELCOME_SELECT_THEME_LIGHT_LABEL},
+    {"braveWelcomeSelectThemeDarkLabel",
+     IDS_BRAVE_WELCOME_SELECT_THEME_DARK_LABEL}};
 
 void OpenJapanWelcomePage(Profile* profile) {
   DCHECK(profile);
@@ -121,7 +126,6 @@ class WelcomeDOMHandler : public WebUIMessageHandler {
   void SetP3AEnabled(const base::Value::List& args);
   void HandleOpenSettingsPage(const base::Value::List& args);
   void HandleSetMetricsReportingEnabled(const base::Value::List& args);
-  void SetBraveThemeType(const base::Value::List& args);
   Browser* GetBrowser();
 
   int screen_number_ = 0;
@@ -159,10 +163,6 @@ void WelcomeDOMHandler::RegisterMessages() {
       "setMetricsReportingEnabled",
       base::BindRepeating(&WelcomeDOMHandler::HandleSetMetricsReportingEnabled,
                           base::Unretained(this)));
-  web_ui()->RegisterMessageCallback(
-      "setBraveThemeType",
-      base::BindRepeating(&WelcomeDOMHandler::SetBraveThemeType,
-                          base::Unretained(this)));
 }
 
 void WelcomeDOMHandler::HandleImportNowRequested(
@@ -203,16 +203,6 @@ void WelcomeDOMHandler::HandleSetMetricsReportingEnabled(
       enabled, ChangeMetricsReportingStateCalledFrom::kUiSettings);
 }
 
-void WelcomeDOMHandler::SetBraveThemeType(const base::Value::List& args) {
-  CHECK_EQ(args.size(), 1U);
-  CHECK(args[0].is_int());
-  AllowJavascript();
-
-  int int_type = args[0].GetInt();
-  dark_mode::SetBraveDarkModeType(
-      static_cast<dark_mode::BraveDarkModeType>(int_type));
-}
-
 void WelcomeDOMHandler::SetLocalStateBooleanEnabled(
     const std::string& path,
     const base::Value::List& args) {

diff --git a/components/brave_welcome_ui/api/welcome_browser_proxy.ts b/components/brave_welcome_ui/api/welcome_browser_proxy.ts
@@ -31,7 +31,6 @@ export interface WelcomeBrowserProxy {
   setP3AEnabled: (enabled: boolean) => void
   setMetricsReportingEnabled: (enabled: boolean) => void
   openSettingsPage: () => void
-  setBraveThemeType: (value: number) => void
 }
 
 export { DefaultBrowserBrowserProxyImpl, ImportDataBrowserProxyImpl }
@@ -53,10 +52,6 @@ export class WelcomeBrowserProxyImpl implements WelcomeBrowserProxy {
     chrome.send('openSettingsPage')
   }
 
-  setBraveThemeType (value: number) {
-    chrome.send('setBraveThemeType', [value])
-  }
-
   static getInstance (): WelcomeBrowserProxy {
     return instance || (instance = new WelcomeBrowserProxyImpl())
   }

diff --git a/components/brave_welcome_ui/components/select-theme/index.tsx b/components/brave_welcome_ui/components/select-theme/index.tsx
@@ -10,22 +10,20 @@ import { getLocale } from '$web-common/locale'
 import classnames from '$web-common/classnames'
 import Button from '$web-components/button'
 
-import { WelcomeBrowserProxyImpl } from '../../api/welcome_browser_proxy'
-
 import DataContext from '../../state/context'
 import { ViewType } from '../../state/component_types'
 
 interface ThemeModeItemProps {
-  themeType: number
+  themeType: chrome.braveTheme.ThemeType
   title: string
   isActive: boolean
-  onChange?: (themeType: number) => void
+  onChange?: (themeType: chrome.braveTheme.ThemeType) => void
 }
 
 const themeList = [
-  { themeType: 0, title: 'Match system setting' },
-  { themeType: 2, title: 'Light mode' },
-  { themeType: 1, title: 'Dark mode' }
+  { themeType: 'System', title: getLocale('braveWelcomeSelectThemeSystemLabel') },
+  { themeType: 'Light', title: getLocale('braveWelcomeSelectThemeLightLabel') },
+  { themeType: 'Dark', title: getLocale('braveWelcomeSelectThemeDarkLabel') }
 ]
 
 function ThemeModeItem (props: ThemeModeItemProps) {
@@ -39,9 +37,9 @@ function ThemeModeItem (props: ThemeModeItemProps) {
   })
 
   const logoBoxClass = classnames({
-    'logo-box': true,
-    'dark-mode': props.themeType === 1,
-    'system-mode': props.themeType === 0
+    'logo': true,
+    'dark-mode': props.themeType === 'Dark',
+    'system-mode': props.themeType === 'System'
   })
 
   return (
@@ -55,17 +53,19 @@ function ThemeModeItem (props: ThemeModeItemProps) {
           </svg>
         )}
       </i>
-      <div className={logoBoxClass} />
-      <p className="theme-name">{props.title}</p>
+      <div className="logo-box">
+        <div className={logoBoxClass} />
+        <p className="theme-name">{props.title}</p>
+      </div>
     </button>
   )
 }
 
 function SelectTheme () {
   const { setViewType, scenes } = React.useContext(DataContext)
-  const [currentSelectedTheme, setCurrentTheme] = React.useState(0)
+  const [currentSelectedTheme, setCurrentTheme] = React.useState<chrome.braveTheme.ThemeType>('System')
 
-  const handleSelectionChange = (themeType: number) => {
+  const handleSelectionChange = (themeType: chrome.braveTheme.ThemeType) => {
     setCurrentTheme?.(themeType)
   }
 
@@ -75,7 +75,7 @@ function SelectTheme () {
   }
 
   const handleNext = () => {
-    WelcomeBrowserProxyImpl.getInstance().setBraveThemeType(currentSelectedTheme)
+    chrome.braveTheme.setBraveThemeType(currentSelectedTheme)
     setViewType(ViewType.HelpImprove)
     scenes?.s2.play()
   }
@@ -92,14 +92,14 @@ function SelectTheme () {
       <S.ThemeListBox>
         <div className={classnames({
           'theme-list': true,
-          'is-selected-dark': currentSelectedTheme === 1,
-          'is-selected-light': currentSelectedTheme === 2
+          'is-selected-dark': currentSelectedTheme === 'Dark',
+          'is-selected-light': currentSelectedTheme === 'Light'
         })}>
           {themeList.map((entry, id) => {
             return (
               <ThemeModeItem
                 key={id}
-                themeType={entry.themeType}
+                themeType={entry.themeType as chrome.braveTheme.ThemeType}
                 title={entry.title}
                 isActive={entry.themeType === currentSelectedTheme}
                 onChange={handleSelectionChange}

diff --git a/components/brave_welcome_ui/components/select-theme/style.ts b/components/brave_welcome_ui/components/select-theme/style.ts
@@ -107,17 +107,26 @@ export const ThemeListBox = styled.div`
   .theme-name {
     font-weight: 400;
     font-size: 14px;
+    line-height: 1.2;
     margin: 0;
+    min-height: calc(2 * 14px * 1.2); // A min height of 2 lines
   }
 
   .logo-box {
+    display: grid;
+    grid-template-rows: 50px 1fr;
+    gap: 10px;
+    width: 100%;
+  }
+
+  .logo {
     width: 48px;
     height: 48px;
     border-radius: 48px;
-    margin-bottom: 10px;
     background: var(--background-color-light);
     border: 1px solid #C2C4CF;
     position: relative;
+    margin: 0 auto;
 
     &.dark-mode {
       background: var(--background-color-dark);

diff --git a/components/brave_welcome_ui/stories/locale.ts b/components/brave_welcome_ui/stories/locale.ts
@@ -27,5 +27,8 @@ provideStrings({
   braveWelcomeChangeSettingsNote: ' Change these choices at any time in Brave at $1brave://settings/privacy$2.',
   braveWelcomePrivacyPolicyNote: 'Read our full $1Privacy Policy$2',
   braveWelcomeSelectThemeLabel: 'Choose your theme',
-  braveWelcomeSelectThemeNote: 'You can change this at any time in Brave settings.'
+  braveWelcomeSelectThemeNote: 'You can change this at any time in Brave settings.',
+  braveWelcomeSelectThemeSystemLabel: 'Match system setting',
+  braveWelcomeSelectThemeLightLabel: 'Light mode',
+  braveWelcomeSelectThemeDarkLabel: 'Dark mode'
 })
diff --git a/components/resources/brave_welcome_strings.grdp b/components/resources/brave_welcome_strings.grdp
@@ -87,4 +87,16 @@
 <message name="IDS_BRAVE_WELCOME_SELECT_THEME_NOTE" desc="A note for user to change theme in settings">
   You can change this at any time in Brave settings.
 </message>
+
+<message name="IDS_BRAVE_WELCOME_SELECT_THEME_SYSTEM_LABEL" desc="A label to match system setting">
+  Match system setting
+</message>
+
+<message name="IDS_BRAVE_WELCOME_SELECT_THEME_LIGHT_LABEL" desc="A label for light theme setting">
+  Light mode
+</message>
+
+<message name="IDS_BRAVE_WELCOME_SELECT_THEME_DARK_LABEL" desc="A label for dark theme setting">
+  Dark mode
+</message>
 </grit-part>