SwitchControl text now toggles checked state, and is consistently used #10470

petemill · 2017-08-14T07:32:38Z

Test Plan

Open the Clear Browsing Data model or the Brave extension panel
- verify that clicking a switch label causes the checked state to toggle
- verify no visual regressions with switches and text
Open preferences
- test switch controls and text labels on all tabs and dialogs
Open a new tab and navigate to any Url
Open the 'find in page' dialog
- check the 'case sensitivity' switch
- verify that the switch and label are operating the toggle
- verify that the tab theme colors are being used for the switch text label

Supporting text click on switchControl

The click event will now fire from the label, whether it is to the left, right, or top of the switch element.

This also converts text from <span /> to <label /> for accessibility and more appropriate renderer defaults (the cursor) to convey that clicking the label will toggle the checked state.

Usages this affects:

all items in the Brave extension panel (compact and full mode)
the Clear Browsing Data model
the MessageBox component
about:adblock

This is a partial resolution only for #8243 since many usages do not use the built-in switchControl label element but render their own text separately. Changing some of those usages to the switchControl label element causes some minor visual regression which may warrant some discussion. A subsequent PR will address converting these usages, which are not covered by this PR:

most switches in all sections of about:preferences
case-sensitivity switch in the findPage dialog

Ensuring SwitchControl text prop is provided across usages

This ensures all remaining uses of SwitchControl gain the built-in text label click handler.

Covers:

the case-sensitivity toggle in findbar
the switches on all the sections of about:preferences page

I refactored a lot of the SettingCheckbox component as it was duplicating quite a bit of the SwitchControl styles even though it was including the common component, which was creating style conflicts when using the text label feature of SwitchControl - a result of integrating the fix for #8243.

I created a separate PR for this as it involves changes to multiple components which consumed SwitchControl in different ways. If that is too granular then I'm happy to combine these changes with #10468.

I'm aware of #7321 which calls for a larger refactor of switchControl and can be a good opportunity to create a cleaner version of this component whilst analyzing the different state variations now required from the varied uses I've come across. I made sure usages of the component are more consistent, but did not attempt a clean-up of the existing version of the component because of that upcoming issue.

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.

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

…ed state Partial resolution for brave#8243 since many usages do not use the built-in text but render their own text separately. A subsequent commit will address converting these usages. This also converts text from span to label for accessbility and more appropriate renderer defaults (the cursor).

luixxiul · 2017-08-14T07:53:15Z

less/about/preferences.less

-    }
-  }
-}
-


Is it OK to remove them all? Was &.disabled for example converted anywhere?

@luixxiul all the removed styles that were in SettingsCheckBox were already in switchControls.less. The one that wasn't is the space between the label and the switch element. See description for question on whether this should now match all other uses of SwitchControl in the app.

The one that wasn't is the space between the label and the switch element.

Why don't we re-add the space for now until we find a better way of handling the padding than we have currently, avoiding the visual regression?

My idea:

remove padding from the toggle switch (on the very same reason we did remove the margin from browserButton: the switch as such does not require the margin/padding. It gets required only other elements appear around the switch)

add Xch (0.5 - 1ch) to the label

add margin to SettingList in which the switch and the label are wrapped

That is out of scope of this PR so you would not need to address them

Previously the text was rendered with a label element outside the SwitchControl. This made it easier to have custom spacing without affecting the switchControl css, but it broke the label being able to toggle the switch. Now that the SettingsCheckbox (and all usages of that) is passing the text to SwitchControl for labels, the style would have to either be part of SwitchControl or a style override from the usage location, adding to the fragmented style definition problem for the component, that will have to be resolved or integrated in a future refactor. Note that not all uses of SettingsCheckbox have this spacing (see the Payments tab), so this spacing is very specific to a couple of places. Happy to do that if it can't be resolved now, but thought I'd try just in case it was a simple ok in order to avoid the fragmentation.

That sounds good to me. Would you mind adding TODO comment to the source code about the visual regression? I think we should not tackle the issue with this PR. I am pretty sure that normalizing SwitchControl requires massive tasks (including adding them to about:styles) and it blocks this PR to be merged for a while.

If, however, no visual regressions are allowed, overriding with !important will be required.

I took another look at it and added a prop to SettingCheckbox that enables 'compact' styles (i.e. no gap). So by default, the gap is there. So, there should not be a visual regression anymore as long as 773dbb1 is ok

luixxiul · 2017-08-14T07:53:57Z

app/renderer/components/main/findbar.js

@@ -275,16 +279,15 @@ class FindBar extends React.Component {
          id='caseSensitivityCheckbox'
          checkedOn={this.props.isCaseSensitive}
          onClick={this.onCaseSensitivityChange}
-        />
-        <label
-          htmlFor='caseSensitivityCheckbox'


Is it OK to remove this?

The label is now being rendered by the switchControl, and the findBarTestStyle style overrides have been changed to aphrodite-style findBarTextStyles css.

luixxiul · 2017-08-14T07:54:20Z

app/renderer/components/common/settings.js

-      <label className={labelClass}
-        data-l10n-id={this.props.dataL10nId}
-        data-l10n-args={this.props.dataL10nArgs}
-        htmlFor={this.props.prefKey}


luixxiul · 2017-08-14T07:55:53Z

less/about/preferences.less

-    }
-  }
-
-  &.large {


ok to remove this?

luixxiul · 2017-08-14T07:56:17Z

less/switchControls.less

@@ -36,6 +36,19 @@
      }
    }
  }
+
+  &.small {


This change affects every switch controls outside of about:preferences too. did you confirm this does not regress something?

In my search, no other component currently uses the small variant apart from about:preferences, this moves the previous style override to the shared component as the large variant already was.

luixxiul

I left initial feedback comments. would you mind adding comments to removed lines on LESS files to make sure the removal is actually safe? thanks!

luixxiul · 2017-08-14T08:04:31Z

@petemill does the change not regress switches on about:adblock?

petemill · 2017-08-14T08:13:16Z

@luixxiul no, about:adblock does not use the SettingCheckbox as it uses SwitchControl directly, so it simply benefitted from #10468

and their text labels Adds 'compact' prop to SettingCheckbox. By default, the gap is there since that is the most common state. As per review of brave#10470

luixxiul

I tested the PR manually and the change looks good to me. Would you mind squashing the last two commits and, if necessary, reword the commit messages?

NejcZdovc · 2017-08-14T16:44:16Z

@petemill can we close #10468 in favour of this one?

luixxiul · 2017-08-14T17:04:45Z

@petemill I copied the PR message from #10468 for you in case it is closed. Please edit it as you would, thanks!

closes brave#8243 as this ensures all remaining uses of switchControl gain the built-in label click handler. covers the case-sensitivity toggle in findbar, as well as all the switches on the preferences page.

petemill · 2017-08-14T20:40:44Z

@luixxiul I squashed the fixes
@NejcZdovc I closed #10468 in favor of this combined PR and edit the description here

NejcZdovc · 2017-08-14T20:43:02Z

@petemill thank you

codecov-io · 2017-08-14T21:15:53Z

Codecov Report

Merging #10470 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10470      +/-   ##
==========================================
- Coverage   54.09%   54.09%   -0.01%     
==========================================
  Files         245      245              
  Lines       21223    21222       -1     
  Branches     3283     3283              
==========================================
- Hits        11481    11480       -1     
  Misses       9742     9742

Flag	Coverage Δ
#unittest	`54.09% <100%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
...erer/components/preferences/payment/ledgerTable.js	`89.5% <ø> (-0.06%)`	⬇️
app/renderer/components/common/settings.js	`79.45% <100%> (ø)`	⬆️
app/renderer/components/preferences/paymentsTab.js	`83.51% <100%> (ø)`	⬆️
app/renderer/components/common/switchControl.js	`96.55% <100%> (ø)`	⬆️

bsclifton · 2017-08-15T23:32:15Z

Great job on this one, @petemill! 😄 👍

petemill mentioned this pull request Aug 14, 2017

SwitchControl text now toggles checked state #10468

Closed

8 tasks

luixxiul reviewed Aug 14, 2017

View reviewed changes

less/about/preferences.less

}

}

&.large {

Copy link

Contributor

luixxiul Aug 14, 2017

Choose a reason for hiding this comment

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

ok to remove this?

luixxiul reviewed Aug 14, 2017

View reviewed changes

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

luixxiul assigned petemill Aug 14, 2017

luixxiul added PR/work-in-progress ⚒ usability labels Aug 14, 2017

luixxiul added the refactoring label Aug 14, 2017

luixxiul removed the PR/work-in-progress ⚒ label Aug 14, 2017

luixxiul approved these changes Aug 14, 2017

View reviewed changes

NejcZdovc requested review from cezaraugusto and NejcZdovc August 14, 2017 16:46

jonathansampson mentioned this pull request Aug 14, 2017

Clicking labels should trigger click of associated element #8243

Closed

petemill changed the title ~~Use SwitchControl text properties instead of sibling label element~~ SwitchControl text now toggles checked state, and is consistently used Aug 14, 2017

Use switchControl text properties instead of sibling label elements

b0460f4

closes brave#8243 as this ensures all remaining uses of switchControl gain the built-in label click handler. covers the case-sensitivity toggle in findbar, as well as all the switches on the preferences page.

petemill force-pushed the switch-control-merge branch from 773dbb1 to b0460f4 Compare August 14, 2017 20:35

luixxiul merged commit 27bc3a8 into brave:master Aug 15, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SwitchControl text now toggles checked state, and is consistently used #10470

SwitchControl text now toggles checked state, and is consistently used #10470

petemill commented Aug 14, 2017 •

edited by luixxiul

Loading

luixxiul Aug 14, 2017

petemill Aug 14, 2017

luixxiul Aug 14, 2017 •

edited

Loading

petemill Aug 14, 2017

luixxiul Aug 14, 2017 •

edited

Loading

petemill Aug 14, 2017

luixxiul Aug 14, 2017

petemill Aug 14, 2017

luixxiul Aug 14, 2017

luixxiul Aug 14, 2017

luixxiul Aug 14, 2017

luixxiul Aug 14, 2017

petemill Aug 14, 2017

luixxiul left a comment

luixxiul commented Aug 14, 2017

petemill commented Aug 14, 2017 •

edited

Loading

luixxiul left a comment •

edited

Loading

NejcZdovc commented Aug 14, 2017

luixxiul commented Aug 14, 2017

petemill commented Aug 14, 2017

NejcZdovc commented Aug 14, 2017

codecov-io commented Aug 14, 2017

bsclifton commented Aug 15, 2017

SwitchControl text now toggles checked state, and is consistently used #10470

SwitchControl text now toggles checked state, and is consistently used #10470

Conversation

petemill commented Aug 14, 2017 • edited by luixxiul Loading

Test Plan

Supporting text click on switchControl

Ensuring SwitchControl text prop is provided across usages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul Aug 14, 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

luixxiul left a comment

Choose a reason for hiding this comment

luixxiul commented Aug 14, 2017

petemill commented Aug 14, 2017 • edited Loading

luixxiul left a comment • edited Loading

Choose a reason for hiding this comment

NejcZdovc commented Aug 14, 2017

luixxiul commented Aug 14, 2017

petemill commented Aug 14, 2017

NejcZdovc commented Aug 14, 2017

codecov-io commented Aug 14, 2017

Codecov Report

bsclifton commented Aug 15, 2017

petemill commented Aug 14, 2017 •

edited by luixxiul

Loading

luixxiul Aug 14, 2017 •

edited

Loading

luixxiul Aug 14, 2017 •

edited

Loading

petemill commented Aug 14, 2017 •

edited

Loading

luixxiul left a comment •

edited

Loading