Add configurable UI scaling #8419

bsclifton · 2017-04-21T07:36:47Z

Break `Preferences > Advanced` into its own file

Auditors: @cezaraugusto, @NejcZdovc

Test Plan:

Open preferences and visit the Advanced tab
Settings should work as expected; there should be no console errors

Add configurable UI scaling

NOTE: This doesn't affect page size; scaling is only for the browser UI elements

Fixes #1937
Fixes #4050

Auditors: @bradleyrichter, @alexwykoff, @NejcZdovc

Test Plan:

Open preferences and visit the Advanced tab
Change the new Toolbar and UI elements scale setting.

verify default is Normal
choose a smaller size and verify URL bar, nav buttons, etc get smaller
choose a larger size and verify URL bar, nav buttons, etc get larger
verify size of page content does not change

Open multiple windows and verify changing the setting takes effect
Verify setting affects ALL UI elements... such as Alert
Verify setting is saved on exit and that all windows are created on launch with the proper size

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).

Screenshots

New preference (under advanced)

The setting in action

NejcZdovc

I love this one

NejcZdovc · 2017-04-21T09:00:45Z

app/common/constants/toolbarUserInterfaceScale.js

+  SUPERSIZE: 'supersize'
+}
+
+let zoomLevel = {}


Shouldn't here be array and not object?

Object makes sense IMO- I use the array style syntax for accessing the object since the dot notation is not available with a variable. ex: zoomLevel.SUPERSIZE won't work, hence zoomLevel[SUPERSIZE]

NejcZdovc · 2017-04-21T09:01:40Z

app/renderer/components/preferences/advancedTab.js

+            value={getSetting(settings.TOOLBAR_UI_SCALE, this.props.settings)}
+            onChange={changeSetting.bind(null, this.props.onChangeSetting, settings.TOOLBAR_UI_SCALE)}>
+            data-type='float'>
+            <option value={scaleSize.SMALLER}>Smaller</option>


Please translate this values

GREAT catch!! 😄

NejcZdovc · 2017-04-21T09:01:57Z

app/renderer/components/preferences/advancedTab.js

+
+  moreInfo: {
+    display: 'flex',
+    flex: '1',


You don't need string here

NejcZdovc · 2017-04-21T09:04:30Z

app/renderer/components/preferences/advancedTab.js

+const React = require('react')
+const ImmutableComponent = require('../immutableComponent')
+
+const {SettingsList, SettingItem, SettingCheckbox} = require('../settings')


Can you please reorder imports in something like this: https://github.com/brave/browser-laptop/blob/master/js/components/main.js or https://github.com/brave/browser-laptop/pull/8225/files#diff-169b96dac6a9688438a29c5013735bda

This way it's really easy to find if you are searching for something.

NejcZdovc · 2017-04-21T09:05:13Z

app/renderer/components/preferences/advancedTab.js

@@ -0,0 +1,61 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


+++++ On splitting into a new file ❤️

luixxiul · 2017-04-21T09:10:24Z

app/renderer/components/preferences/advancedTab.js

+
+const {SettingsList, SettingItem, SettingCheckbox} = require('../settings')
+const {SettingDropdown} = require('../dropdown')
+const {StyleSheet, css} = require('aphrodite')


Let's add no-important to it. CC @cezaraugusto

++ no-important pls
const {StyleSheet, css} = require('aphrodite/no-important')

cezaraugusto · 2017-04-21T16:19:08Z

app/renderer/components/preferences/advancedTab.js

+        </SettingItem>
+      </main>
+      <footer className={css(styles.moreInfo)}>
+        <div data-l10n-id='requiresRestart' className='requiresRestart' />


can we put requiresRestart inside commonStyles.js?

bsclifton · 2017-04-22T07:25:30Z

@bradleyrichter, @NejcZdovc, @cezaraugusto, @luixxiul Feedback incorporated! Give it a spin, re-review, let me know what you think 😄

cndouglas · 2017-04-22T17:03:30Z

@bsclifton There are some conflicts in js/about/preferences.js.

bsclifton · 2017-04-23T09:31:13Z

@liunkae thanks for the heads up- rebased 😄

NejcZdovc · 2017-04-23T21:13:17Z

app/renderer/components/preferences/advancedTab.js

+
+class AdvancedTab extends ImmutableComponent {
+  render () {
+    return <div>


Can you please redo this page so it will be aligned with this new approach that was added in this PR #8237

NejcZdovc · 2017-04-23T21:15:00Z

app/renderer/components/preferences/advancedTab.js

+  render () {
+    return <div>
+      <main className={css(styles.advancedTabMain)}>
+        <div className='sectionTitle' data-l10n-id='contentSettings' />


Basically you need to change this line in the same was @luixxiul did it https://github.com/brave/browser-laptop/pull/8237/files#diff-e3eeb751016b2ce9f8278efce585a461L659

NejcZdovc · 2017-04-23T21:16:04Z

app/renderer/components/preferences/advancedTab.js

+          <SettingCheckbox dataL10nId='sendUsageStatistics' prefKey={settings.SEND_USAGE_STATISTICS} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
+        </SettingsList>
+
+        <div className='sectionTitle' data-l10n-id='toolbarUserInterfaceScale' />


@cezaraugusto

…etting there Auditors: @cezaraugusto, @NejcZdovc Test Plan: 1. Open preferences and visit the `Advanced` tab 2. Settings should work as expected; there should be no console errors Add configurable UI scaling. This doesn't affect page size; scaling is only for the browser UI elements Fixes #1937 Auditors: @bradleyrichter, @alexwykoff, @NejcZdovc Test Plan: 1. Open preferences and visit the `Advanced` tab 2. Change the new `Toolbar and UI elements scale` setting. - verify default is `Normal` - choose a smaller size and verify URL bar, nav buttons, etc get smaller - choose a larger size and verify URL bar, nav buttons, etc get larger - verify size of page content does not change This commit incorporates feedback from PR - organize imports - translations for the new drop down values - moved "requires restart" style to commonStyles - switch advancedTab to use aphrodite/no-important - updated zoom values per convo w/ @bradleyrichter - use new `DefaultSectionTitle` tag for the new AdvancedTab component

bsclifton · 2017-04-24T05:53:49Z

@NejcZdovc updated! 😄

NejcZdovc

LGTM now

cezaraugusto

this PR is in a higher ground ♫ of quality ++

bsclifton · 2017-04-24T21:07:41Z

🎹

bsclifton added accessibility design A design change, especially one which needs input from the design team. QA/test-plan-specified release-notes/include labels Apr 21, 2017

bsclifton added this to the 0.15.1 milestone Apr 21, 2017

bsclifton self-assigned this Apr 21, 2017

bsclifton requested review from alexwykoff, cezaraugusto, NejcZdovc and bradleyrichter April 21, 2017 07:36

NejcZdovc suggested changes Apr 21, 2017

View reviewed changes

luixxiul reviewed Apr 21, 2017

View reviewed changes

cezaraugusto reviewed Apr 21, 2017

View reviewed changes

bsclifton force-pushed the zoom-for-ui-elements branch from 35175ce to cef0d2a Compare April 22, 2017 07:23

luixxiul added the PR/needs-rebase ۞ label Apr 23, 2017

bbondy force-pushed the master branch from f0df59d to 70e5d77 Compare April 23, 2017 04:21

bsclifton force-pushed the zoom-for-ui-elements branch from cef0d2a to 046af3f Compare April 23, 2017 09:29

bsclifton removed the PR/needs-rebase ۞ label Apr 23, 2017

NejcZdovc reviewed Apr 23, 2017

View reviewed changes

bsclifton force-pushed the zoom-for-ui-elements branch from 046af3f to 6527c55 Compare April 24, 2017 05:53

NejcZdovc approved these changes Apr 24, 2017

View reviewed changes

cezaraugusto approved these changes Apr 24, 2017

View reviewed changes

cezaraugusto merged commit 2212b33 into master Apr 24, 2017

bsclifton deleted the zoom-for-ui-elements branch April 24, 2017 21:07

luixxiul removed QA/test-plan-specified release-notes/include labels Apr 26, 2017

luixxiul mentioned this pull request Apr 30, 2017

Increase bookmark text size #4050

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configurable UI scaling #8419

Add configurable UI scaling #8419

bsclifton commented Apr 21, 2017 •

edited by luixxiul

Loading

NejcZdovc left a comment

NejcZdovc Apr 21, 2017

bsclifton Apr 21, 2017

NejcZdovc Apr 21, 2017

NejcZdovc Apr 21, 2017

bsclifton Apr 21, 2017

NejcZdovc Apr 21, 2017

NejcZdovc Apr 21, 2017

NejcZdovc Apr 21, 2017

luixxiul Apr 21, 2017

cezaraugusto Apr 21, 2017

cezaraugusto Apr 21, 2017

bsclifton commented Apr 22, 2017

cndouglas commented Apr 22, 2017

bsclifton commented Apr 23, 2017

NejcZdovc Apr 23, 2017

NejcZdovc Apr 23, 2017

NejcZdovc Apr 23, 2017

bsclifton commented Apr 24, 2017

NejcZdovc left a comment

cezaraugusto left a comment

bsclifton commented Apr 24, 2017

		@@ -0,0 +1,61 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

Add configurable UI scaling #8419

Add configurable UI scaling #8419

Conversation

bsclifton commented Apr 21, 2017 • edited by luixxiul Loading

Break Preferences > Advanced into its own file

Test Plan:

Add configurable UI scaling

Test Plan:

Screenshots

NejcZdovc left a 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

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

bsclifton commented Apr 22, 2017

cndouglas commented Apr 22, 2017

bsclifton commented Apr 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Apr 24, 2017

NejcZdovc left a comment

Choose a reason for hiding this comment

cezaraugusto left a comment

Choose a reason for hiding this comment

bsclifton commented Apr 24, 2017

bsclifton commented Apr 21, 2017 •

edited by luixxiul

Loading

Break `Preferences > Advanced` into its own file