Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

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

Merged
merged 2 commits into from
Aug 15, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7778d7
SwitchControl text to left, right or top of control now toggles check…
petemill Aug 14, 2017
b0460f4
Use switchControl text properties instead of sibling label elements
petemill Aug 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions app/renderer/components/common/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ class SettingCheckbox extends ImmutableComponent {

const labelClass = cx({
[css(settingCheckboxStyles.label)]: this.props.small,
[this.props.labelClassName]: !!this.props.labelClassName

[this.props.labelClassName]: !!this.props.labelClassName,
[css(settingCheckboxStyles.expansiveRightText)]: !this.props.compact
})

return <div {...props} data-test-id={this.props.dataTestId}>
Expand All @@ -101,11 +101,9 @@ class SettingCheckbox extends ImmutableComponent {
onClick={this.onClick}
checkedOn={this.props.checked !== undefined ? this.props.checked : getSetting(this.props.prefKey, this.props.settings)}
className={this.props.switchClassName}
/>
<label className={labelClass}
data-l10n-id={this.props.dataL10nId}
data-l10n-args={this.props.dataL10nArgs}
htmlFor={this.props.prefKey}
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

customRightTextClassName={labelClass}
rightl10nId={this.props.dataL10nId}
rightl10nArgs={this.props.dataL10nArgs}
/>
{this.props.options}
</div>
Expand All @@ -115,6 +113,9 @@ class SettingCheckbox extends ImmutableComponent {
const settingCheckboxStyles = StyleSheet.create({
label: {
fontSize: 'smaller'
},
expansiveRightText: {
paddingLeft: '9px'
}
})

Expand Down
21 changes: 12 additions & 9 deletions app/renderer/components/common/switchControl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,19 @@ class SwitchControl extends ImmutableComponent {
>
{
this.props.leftl10nId && this.props.topl10nId
? <div className='switchControlText'><div className='switchControlLeftText'><div className='switchSpacer'>&nbsp;</div><span className='switchControlLeftText' data-l10n-id={this.props.leftl10nId} /></div></div>
? <div className='switchControlText'><div className='switchControlLeftText'><div className='switchSpacer'>&nbsp;</div><label className='switchControlLeftText' onClick={this.onClick} data-l10n-id={this.props.leftl10nId} /></div></div>
: (this.props.leftl10nId
? <span className='switchControlLeftText' data-l10n-id={this.props.leftl10nId} />
? <label className='switchControlLeftText' onClick={this.onClick} data-l10n-id={this.props.leftl10nId} />
: null)
}
<div className='switchMiddle'>
{
this.props.topl10nId
? <span className={cx({
? <label className={cx({
switchControlTopText: true,
[this.props.customTopTextClassName]: !!this.props.customTopTextClassName
})}
onClick={this.onClick}
data-l10n-id={this.props.topl10nId} />
: null
}
Expand All @@ -67,34 +68,36 @@ class SwitchControl extends ImmutableComponent {
? <div className='switchControlText'>
<div className='switchControlRightText'>
<div className='switchSpacer'>&nbsp;</div>
<span className={cx({
<label className={cx({
switchControlRightText: true,
[this.props.customRightTextClassName]: !!this.props.customRightTextClassName
})}
onClick={this.onClick}
data-l10n-id={this.props.rightl10nId}
data-l10n-args={this.props.rightl10nArgs}
>{this.props.rightText || ''}</span>
>{this.props.rightText || ''}</label>
</div>
</div>
: <div className='switchControlRight'>
{
(this.props.rightl10nId || this.props.rightText) && !this.props.onInfoClick
? <span className={cx({
? <label className={cx({
switchControlRightText: true,
[this.props.customRightTextClassName]: !!this.props.customRightTextClassName
})}
onClick={this.onClick}
data-l10n-id={this.props.rightl10nId}
data-l10n-args={this.props.rightl10nArgs}
>{this.props.rightText || ''}</span>
>{this.props.rightText || ''}</label>
: null
}
{
(this.props.rightl10nId || this.props.rightText) && this.props.onInfoClick
? <div className='switchControlRightText'>
<span data-l10n-id={this.props.rightl10nId}
<label onClick={this.onClick} data-l10n-id={this.props.rightl10nId}
data-l10n-args={this.props.rightl10nArgs}>
{this.props.rightText}
</span>
</label>
<span className={cx({
fa: true,
'fa-question-circle': true,
Expand Down
25 changes: 14 additions & 11 deletions app/renderer/components/main/findbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

const React = require('react')
const Immutable = require('immutable')
const {StyleSheet, css} = require('aphrodite')

// Components
const ReduxComponent = require('../reduxComponent')
Expand All @@ -24,6 +25,7 @@ const {getTextColorForBackground} = require('../../../../js/lib/color')
const frameStateUtil = require('../../../../js/state/frameStateUtil')
const {getSetting} = require('../../../../js/settings')
const debounce = require('../../../../js/lib/debounce')
const cx = require('../../../../js/lib/classSet')

// Styles
const globalStyles = require('../styles/global')
Expand Down Expand Up @@ -224,16 +226,18 @@ class FindBar extends React.Component {

render () {
let findBarStyle = {}
let findBarTextStyle = {}
let findBarTextStyles = {}

if (this.props.backgroundColor) {
findBarTextStyles = StyleSheet.create({
matchingTextColor: {
color: this.props.textColor
}
})
findBarStyle = {
background: this.props.backgroundColor,
color: this.props.textColor
}
findBarTextStyle = {
color: this.props.textColor
}
}

return <div className='findBar' style={findBarStyle} onBlur={this.onBlur}>
Expand Down Expand Up @@ -275,16 +279,15 @@ class FindBar extends React.Component {
id='caseSensitivityCheckbox'
checkedOn={this.props.isCaseSensitive}
onClick={this.onCaseSensitivityChange}
/>
<label
htmlFor='caseSensitivityCheckbox'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it OK to remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed!

data-l10n-id='caseSensitivity'
style={findBarTextStyle}
customRightTextClassName={css(findBarTextStyles.matchingTextColor)}
rightl10nId='caseSensitivity'
/>
</div>
<span
className='closeButton'
style={findBarTextStyle}
className={cx({
closeButton: true,
[css(findBarTextStyles.matchingTextColor)]: true
})}
onClick={this.onFindHide}
>+</span>
</div>
Expand Down
2 changes: 1 addition & 1 deletion app/renderer/components/preferences/payment/ledgerTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ class LedgerTable extends ImmutableComponent {
<div className={css(styles.hideExcludedSites)}>
<div className={css(styles.columnOffset)} />
<div className={css(styles.rightColumn)}>
<SettingCheckbox small
<SettingCheckbox small compact
dataL10nId='hideExcluded'
prefKey={settings.PAYMENTS_SITES_HIDE_EXCLUDED}
settings={this.props.settings}
Expand Down
19 changes: 6 additions & 13 deletions app/renderer/components/preferences/paymentsTab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,11 @@ class PaymentsTab extends ImmutableComponent {
styles.flexAlignCenter,
styles.paymentsSwitches
)}>
<div className={css(styles.flexAlignEnd, styles.switchWrap)} data-test-id='enablePaymentsSwitch'>
<div className={css(styles.flexAlignCenter, styles.switchWrap)} data-test-id='enablePaymentsSwitch'>
<span className={css(styles.switchWrap__switchSpan)} data-l10n-id='off' />
<SettingCheckbox dataL10nId='on'
<SettingCheckbox
dataL10nId='on'
compact
prefKey={settings.PAYMENTS_ENABLED}
settings={this.props.settings}
onChangeSetting={this.props.onChangeSetting}
Expand Down Expand Up @@ -226,6 +228,7 @@ class PaymentsTab extends ImmutableComponent {
<div className={css(styles.switchWrap__autoSuggestSwitch)}>
<div className={css(styles.flexAlignCenter, styles.autoSuggestSwitch__subtext)}>
<SettingCheckbox dataL10nId='autoSuggestSites'
compact
prefKey={settings.PAYMENTS_SITES_AUTO_SUGGEST}
settings={this.props.settings}
disabled={!enabled}
Expand Down Expand Up @@ -280,10 +283,6 @@ const styles = StyleSheet.create({
display: 'flex',
alignItems: 'center'
},
flexAlignEnd: {
display: 'flex',
alignItems: 'flex-end'
},

paymentsContainer: {
position: 'relative',
Expand All @@ -307,11 +306,6 @@ const styles = StyleSheet.create({
switchWrap: {
width: paymentStyles.width.tableCell
},
switchWrap__switchControl: {
// TODO: Refactor switchControls.less
paddingTop: '0 !important',
paddingBottom: '0 !important'
},
switchWrap__switchSpan: {
color: '#999',
fontWeight: 'bold'
Expand Down Expand Up @@ -340,8 +334,7 @@ const styles = StyleSheet.create({
},
autoSuggestSwitch__moreInfoBtnSuggest: {
position: 'relative',
top: '-1px',
left: '8px',
left: '3px',
cursor: 'pointer',

// TODO: refactor preferences.less to remove !important
Expand Down
99 changes: 0 additions & 99 deletions less/about/preferences.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,105 +272,6 @@ table.sortableTable {
color: @braveOrange;
}

.prefTabContainer .switchControl {
display: inline-block;
vertical-align: middle;
align-items: center;
padding: 5px;
cursor: pointer;

&.disabled {
.switchBackground {
opacity: 0.3;
}
.switchControlRightText, .switchControlLeftText {
opacity: 0.3;
}
cursor: default;
}

&:not(.disabled) {
.switchBackground.switchedOn {
background-color: @switchBG_on;
.switchIndicator {
right: 2px;
}
}
}

&.large {
Copy link
Contributor

Choose a reason for hiding this comment

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

ok to remove this?

.switchBackground {
height: 26px;
width: 60px;
.switchIndicator {
height: 22px;
width: 22px;
}
}
}

&.small {
.switchBackground {
height: @smallSwitchHeight;
width: @smallSwitchWidth;
.switchIndicator {
height: @smallSwitchNubDiameter;
width: @smallSwitchNubDiameter;
top: 1px;
right: calc(@smallSwitchWidth - @smallSwitchNubDiameter - @switchNubRightMargin);
}
}
}

> label {
vertical-align: middle;
}

.switchControlText {
margin: auto 0;
}

.switchControlLeftText {
margin: auto 5px auto 0;
}

.switchControlRightText {
margin: auto 0 auto 5px;
}

.switchControlTopText {
margin: 0 auto 5px auto;
}

.switchControlRight {
display: flex;
}

.switchMiddle {
display: flex;
flex-direction: column;
}

.switchBackground {
background-color: @switchBG_off;
border-radius: 12px;
height: @switchHeight;
position: relative;
width: @switchWidth;
box-shadow: @switchShadow;

.switchIndicator {
background-color: white;
border-radius: 100%;
height: @switchNubDiameter;
position: absolute;
top: @switchNubTopMargin;
width: @switchNubDiameter;
box-shadow: @switchNubShadow;
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@luixxiul luixxiul Aug 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. 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)
  2. add Xch (0.5 - 1ch) to the label
  3. 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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@luixxiul luixxiul Aug 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

.widevineInfo {
line-height: 1.3em;

Expand Down
13 changes: 13 additions & 0 deletions less/switchControls.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,19 @@
}
}
}

&.small {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

.switchBackground {
height: @smallSwitchHeight;
width: @smallSwitchWidth;
.switchIndicator {
height: @smallSwitchNubDiameter;
width: @smallSwitchNubDiameter;
top: 1px;
right: calc(@smallSwitchWidth - @smallSwitchNubDiameter - @switchNubRightMargin);
}
}
}

.switchControlText {
margin: auto 0;
Expand Down