Partial refactoring of switchControl.js #10482

luixxiul · 2017-08-15T02:15:06Z

Addresses #7321
~~Fixes #11937~~

Add rightLabelClassName and leftLabelClassName props
Add user-select:none to the labels
Move 'small' option from settings.js to switchControls.js, removing settingsCheckboxStyles
Remove 'compact' props as padding-left / padding-right is set by default
Replace 9px margin with 1ch padding for labels
Overwrite that padding with 0.75ch for payments switch labels

TODO: Apply 'position: relative' and 'bottom: 1px' for macOS (en_US) only on about:preferences#payments.

The placement of the right label has been corrupted on the other environments such as Windows 10 (ja-JP);

Auditors: @cezaraugusto @petemill

Test Plan 1:

Open about:preferences
Make sure there is padding space between switches and labels

Test Plan 2:

Open about:preferences#payments
Make sure the 3 switches are displayed
Change the language setting
Restart the browser
Reopen about:preferences#payments
Make sure the labels and the switch are aligned at the center

Test Plan 3:

Open braveryPanel.js
Replace 'SwitchControl large' to 'SwitchControl small'
Run npm run watch && npm start
Open brave.com
Open the bravery panel
Make sure the main switch and the labels around it are small

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.

Test Plan:

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

luixxiul · 2017-08-15T02:16:42Z

app/renderer/components/preferences/paymentsTab.js

@@ -197,21 +197,21 @@ class PaymentsTab extends ImmutableComponent {
          styles.paymentsSwitches
        )}>
          <div className={css(styles.flexAlignCenter, styles.switchWrap)} data-test-id='enablePaymentsSwitch'>
-            <span className={css(styles.switchWrap__switchSpan)} data-l10n-id='off' />


Replaced with dataL10nIdLeft='off' below.

luixxiul · 2017-08-15T02:19:06Z

app/renderer/components/common/settings.js

      />
      {this.props.options}
    </div>
  }
 }

-const settingCheckboxStyles = StyleSheet.create({


"The Cause of the Fragments"

luixxiul · 2017-08-15T06:02:53Z

app/renderer/components/common/settings.js

-    fontSize: 'smaller'
-  },
-  expansiveRightText: {
-    paddingLeft: '9px'


Deprecated by https://github.com/brave/browser-laptop/pull/10482/files#diff-37d79abec3d2254f619089a4be374d3fR157

luixxiul · 2017-08-15T06:03:16Z

app/renderer/components/preferences/payment/ledgerTable.js

@@ -264,7 +264,8 @@ class LedgerTable extends ImmutableComponent {
      <div className={css(styles.hideExcludedSites)}>
        <div className={css(styles.columnOffset)} />
        <div className={css(styles.rightColumn)}>
-          <SettingCheckbox small compact


Deprecated by https://github.com/brave/browser-laptop/pull/10482/files#diff-692d90dadf26ea0dbbbfa01ed6e62461L94

luixxiul · 2017-08-15T06:03:25Z

app/renderer/components/preferences/paymentsTab.js

            <SettingCheckbox
              dataL10nId='on'
-              compact


luixxiul · 2017-08-15T06:03:33Z

app/renderer/components/preferences/paymentsTab.js

@@ -228,7 +227,6 @@ class PaymentsTab extends ImmutableComponent {
              <div className={css(styles.switchWrap__autoSuggestSwitch)}>
                <div className={css(styles.flexAlignCenter, styles.autoSuggestSwitch__subtext)}>
                  <SettingCheckbox dataL10nId='autoSuggestSites'
-                    compact


codecov-io · 2017-08-15T06:08:53Z

Codecov Report

Merging #10482 into master will decrease coverage by <.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master   #10482      +/-   ##
==========================================
- Coverage   54.24%   54.23%   -0.01%     
==========================================
  Files         249      249              
  Lines       21836    21844       +8     
  Branches     3399     3407       +8     
==========================================
+ Hits        11844    11847       +3     
- Misses       9992     9997       +5

Flag	Coverage Δ
#unittest	`54.23% <88.88%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
...erer/components/preferences/payment/ledgerTable.js	`89.67% <ø> (ø)`	⬆️
app/renderer/components/common/settings.js	`78.57% <ø> (-0.89%)`	⬇️
app/renderer/components/preferences/paymentsTab.js	`83.87% <ø> (ø)`	⬆️
app/renderer/components/common/switchControl.js	`85% <88.88%> (-11.56%)`	⬇️

cezaraugusto · 2017-09-04T23:24:54Z

@luixxiul please rebase

luixxiul · 2017-09-05T03:20:20Z

rebased!

luixxiul · 2017-09-05T04:00:43Z

app/renderer/components/common/switchControl.js

        : (this.props.leftl10nId
-          ? <label className='switchControlLeftText' onClick={this.onClick} data-l10n-id={this.props.leftl10nId} />
+          ? <label className={cx({


Before rebasing this line had been wrapped with div. It was a regression, so I removed that.

cezaraugusto · 2017-09-15T19:44:02Z

app/renderer/components/common/switchControl.js

+          <div className='switchControlLeftText'>
+            <div className='switchSpacer'>&nbsp;</div>
+            <label className={cx({
+              [css(styles.switchControlText__label, styles.switchControlText_left__label, this.props.small && styles.switchControlText__label_small)]: true,


better if

[css(styles.switchControlText__label]: true, [css(styles.switchControlText_left__label): true, [styles.switchControlText__label_small]: this.props.small

lmk if you disagree

If you set background: #000 to switchControlText__label and background: #111 to switchControlText_left__label at the same time, we usually get background: #000 instead of the latter, which is what we want. I'd love to prefer readability but we have to assure that the styles are surely cascaded.

I've noticed that issue for a while and tried another way to set them as follows:

[css( styles.switchControlText__label, styles.switchControlText_left__label )]

but it was avoided because Standard complained about that the [ and ] should appear on the very same line.

Still I believe this is a kind of issue on the transitional phase until you would eventually remove cx by creating components in a way which @petemill has demonstrated.

We could create an issue to track the work.

It seems to me that replacing customLeftTextClassName with this.props.custom as you @cezaraugusto did on browserButton should work. I took that way for commonForm on #10978, e.g. <CommonForm custom={styles.commonForm_small}>, so <SwitchLabel custom={styles.switchControlText_left__label}> or <SwitchLabel left>.

cezaraugusto · 2017-09-15T19:44:45Z

app/renderer/components/preferences/paymentsTab.js

-    fontWeight: 'bold',
+  switchWrap__label_left: {
+    paddingRight: '.75ch !important'
+  },


don't you prefer spacing brakets?

nice catch, thanks!

cezaraugusto · 2017-09-15T19:45:36Z

app/renderer/components/preferences/paymentsTab.js

            />
            <a className={cx({
              fa: true,
              'fa-question-circle': true,
-              [css(styles.autoSuggestSwitch__moreInfo)]: true,
-              [css(styles.autoSuggestSwitch__moreInfoBtnSuggest)]: true
+              [css(styles.autoSuggestSwitch__moreInfo, styles.autoSuggestSwitch__moreInfoBtnSuggest)]: true


same as above better if

[css(styles.autoSuggestSwitch__moreInfo)]: true, [css(styles.autoSuggestSwitch__moreInfoBtnSuggest)]: true

unless aphrodite is buggy in such cases

cezaraugusto · 2017-09-15T19:50:29Z

app/renderer/components/common/switchControl.js

        : (this.props.leftl10nId
-          ? <label className='switchControlLeftText' onClick={this.onClick} data-l10n-id={this.props.leftl10nId} />
+          ? <label className={cx({
+            [css(styles.switchControlText__label, styles.switchControlText_left__label, this.props.small && styles.switchControlText__label_small)]: true,


same as https://github.com/brave/browser-laptop/pull/10482/files#r139234453

cezaraugusto · 2017-09-15T19:50:38Z

app/renderer/components/common/switchControl.js

          : null)
      }
      <div className='switchMiddle'>
        {
          this.props.topl10nId
          ? <label className={cx({
            switchControlTopText: true,
+            [css(styles.switchControlText__label, this.props.small && styles.switchControlText__label_small)]: true,


cezaraugusto · 2017-09-15T19:50:51Z

app/renderer/components/common/switchControl.js

          <div className={cx({
            switchIndicator: true,
+            [css(this.props.small && styles.switchControlText__label_small)]: true,


cezaraugusto · 2017-09-15T19:51:02Z

app/renderer/components/common/switchControl.js

@@ -69,7 +91,7 @@ class SwitchControl extends ImmutableComponent {
          <div className='switchControlRightText'>
            <div className='switchSpacer'>&nbsp;</div>
            <label className={cx({
-              switchControlRightText: true,
+              [css(styles.switchControlText__label, styles.switchControlText_right__label, this.props.small && styles.switchControlText__label_small)]: true,


cezaraugusto · 2017-09-15T19:51:08Z

app/renderer/components/common/switchControl.js

@@ -82,7 +104,7 @@ class SwitchControl extends ImmutableComponent {
          {
            (this.props.rightl10nId || this.props.rightText) && !this.props.onInfoClick
            ? <label className={cx({
-              switchControlRightText: true,
+              [css(styles.switchControlText__label, styles.switchControlText_right__label, this.props.small && styles.switchControlText__label_small)]: true,


cezaraugusto · 2017-09-15T19:51:19Z

app/renderer/components/common/switchControl.js

-                data-l10n-args={this.props.rightl10nArgs}>
+            ? <div className='switchControlRight'>
+              <label className={cx({
+                [css(styles.switchControlText__label, styles.switchControlText_right__label, this.props.small && styles.switchControlText__label_small)]: true,


cezaraugusto · 2017-09-15T19:51:36Z

app/renderer/components/common/switchControl.js

                {this.props.rightText}
              </label>
              <span className={cx({
                fa: true,
                'fa-question-circle': true,
                info: true,
                clickable: true,
+                [css(styles.switchControlText__label, this.props.small && styles.switchControlText__label_small)]: true,


yet another one

Please see above, thanks!

cezaraugusto

lgtm but please see inline comments

Addresses #7321 - Add rightLabelClassName and leftLabelClassName props - Add user-select:none to the labels - Move 'small' option from settings.js to switchControls.js, removing settingsCheckboxStyles - Remove 'compact' props as padding-left / padding-right is set by default - Replace 9px margin with 1ch padding for labels - Overwrite that padding with 0.75ch for payments switch labels TODO: Apply 'position: relative' and 'bottom: 1px' for macOS (en_US) only. The placement of the right label has been corrupted on the other environments such as Windows 10 (ja-JP). Auditors: Test Plan 1: 1. Open about:preferences 2. Make sure there is padding space between switches and labels 3. Make sure the labels text cannot be selected Test Plan 2: 1. Open about:preferences#payments 2. Make sure the 3 switches are displayed 3. Change the language setting 4. Restart the browser 5. Reopen about:preferences#payments 6. Make sure the labels and the switch are aligned at the center Test Plan 3: 1. Open braveryPanel.js 2. Replace 'SwitchControl large' to 'SwitchControl small' 3. Run npm run watch && npm start 4. Open brave.com 5. Open the bravery panel 6. Make sure the main switch and the labels around it are small

cezaraugusto

ok merging. @luixxiul could you please create a tracking issue for cases I mentioned in here? seems to have other cases and I'll in one point move them w/o cx() and add props as you suggested. Nice work!

luixxiul added the refactoring/aphrodite label Aug 15, 2017

luixxiul added this to the 0.21.x (Nightly Channel) milestone Aug 15, 2017

luixxiul self-assigned this Aug 15, 2017

luixxiul requested review from cezaraugusto and bsclifton August 15, 2017 02:15

luixxiul commented Aug 15, 2017

View reviewed changes

app/renderer/components/preferences/paymentsTab.js Outdated

<SettingCheckbox

dataL10nId='on'

compact

Copy link

Contributor Author

luixxiul Aug 15, 2017

Choose a reason for hiding this comment

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

Ditto

luixxiul commented Aug 15, 2017

View reviewed changes

cezaraugusto added the PR/needs-rebase ۞ label Sep 4, 2017

luixxiul removed the PR/needs-rebase ۞ label Sep 5, 2017

luixxiul commented Sep 5, 2017

View reviewed changes

luixxiul added l10n priority/P3 Major loss of function. labels Sep 11, 2017

cezaraugusto reviewed Sep 15, 2017

View reviewed changes

cezaraugusto approved these changes Sep 19, 2017

View reviewed changes

cezaraugusto merged commit acf855c into brave:master Sep 19, 2017

luixxiul removed the priority/P3 Major loss of function. label Sep 20, 2017

luixxiul mentioned this pull request Sep 21, 2017

Remove custom classes from switchControl.js #11065

Closed

luixxiul deleted the switchControl-aphrodite branch September 21, 2017 10:34

bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017

luixxiul mentioned this pull request Nov 13, 2017

adjust 'Allow contributions to videos' toggle text display #11937

Closed

luixxiul mentioned this pull request Nov 21, 2017

Fix BEM naming and align elements correctly on paymentsTab.js and ledgerTable.js #10913

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial refactoring of switchControl.js #10482

Partial refactoring of switchControl.js #10482

luixxiul commented Aug 15, 2017 •

edited

Loading

luixxiul Aug 15, 2017

luixxiul Aug 15, 2017

luixxiul Aug 15, 2017

luixxiul Aug 15, 2017

luixxiul Aug 15, 2017

luixxiul Aug 15, 2017

codecov-io commented Aug 15, 2017 •

edited

Loading

cezaraugusto commented Sep 4, 2017

luixxiul commented Sep 5, 2017

luixxiul Sep 5, 2017

cezaraugusto Sep 15, 2017

luixxiul Sep 15, 2017 •

edited

Loading

luixxiul Sep 16, 2017 •

edited

Loading

cezaraugusto Sep 15, 2017 •

edited

Loading

luixxiul Sep 15, 2017

cezaraugusto Sep 15, 2017

cezaraugusto Sep 15, 2017

cezaraugusto Sep 15, 2017

cezaraugusto Sep 15, 2017

cezaraugusto Sep 15, 2017

cezaraugusto Sep 15, 2017

cezaraugusto Sep 15, 2017

cezaraugusto Sep 15, 2017

luixxiul Sep 15, 2017

cezaraugusto left a comment

cezaraugusto left a comment

Partial refactoring of switchControl.js #10482

Partial refactoring of switchControl.js #10482

Conversation

luixxiul commented Aug 15, 2017 • edited Loading

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

codecov-io commented Aug 15, 2017 • edited Loading

Codecov Report

cezaraugusto commented Sep 4, 2017

luixxiul commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

cezaraugusto Sep 15, 2017 • edited Loading

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

cezaraugusto left a comment

Choose a reason for hiding this comment

cezaraugusto left a comment

Choose a reason for hiding this comment

luixxiul commented Aug 15, 2017 •

edited

Loading

codecov-io commented Aug 15, 2017 •

edited

Loading

luixxiul Sep 15, 2017 •

edited

Loading

luixxiul Sep 16, 2017 •

edited

Loading

cezaraugusto Sep 15, 2017 •

edited

Loading