Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RNMobile] RangeCell styling refactor #18157

Merged
merged 41 commits into from
Nov 15, 2019
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
5f3a400
Revert package-lock.json
lukewalczak Oct 17, 2019
25832f0
Merge branch 'master' into rnmobile/spacer
lukewalczak Oct 17, 2019
335154d
Setting attributes for spacer height
lukewalczak Oct 18, 2019
d137d38
Correct the condition for setting maximum value
lukewalczak Oct 21, 2019
8d37161
Merge branch 'master' into rnmobile/spacer
lukewalczak Oct 21, 2019
ae5715f
Small code refactor
lukewalczak Oct 23, 2019
a1cf07b
Improve Accessibility in range-cell
lukewalczak Oct 23, 2019
f0b3b28
Merge branch 'master' into rnmobile/spacer
lukewalczak Oct 23, 2019
a807c31
More accessibility improvements
lukewalczak Oct 23, 2019
ca3b8d4
Small code refactor
lukewalczak Oct 23, 2019
a390d88
Styling spacer refactor
lukewalczak Oct 24, 2019
90fe027
Move logic to RangeCell
lukewalczak Oct 24, 2019
a1d9a10
Keep Slider along with TextInput within RangeCell
lukewalczak Oct 24, 2019
833cc76
Small cleanup
lukewalczak Oct 24, 2019
103493e
Merge branch 'master' into rnmobile/spacer
lukewalczak Oct 24, 2019
74d020a
Fix missing binds
lukewalczak Oct 24, 2019
8c127b9
Fix focusing slider on iphoneX when VO is on
lukewalczak Oct 28, 2019
84ade89
Adjust a11y voice over
lukewalczak Oct 28, 2019
11e0911
Refactor RangeCell styles
lukewalczak Oct 28, 2019
1debc8c
Refactor pointerEvents when screen reader is on
lukewalczak Oct 28, 2019
4a897fd
Announce current value when finished
lukewalczak Oct 28, 2019
80a4714
Merge branch 'rnmobile/spacer' into rnmobile/rangecell-styling
lukewalczak Oct 28, 2019
047a08e
Small cleanup
lukewalczak Oct 29, 2019
7ee822e
Improve a11y
lukewalczak Oct 29, 2019
6d4dde4
Merge branch 'rnmobile/spacer' into rnmobile/rangecell-styling
lukewalczak Oct 29, 2019
014cd20
Fix a11y on iPhoneX
lukewalczak Oct 30, 2019
9f28b60
Merge branch 'rnmobile/spacer' into rnmobile/rangecell-styling
lukewalczak Oct 30, 2019
2e5c346
Refactor after CR
lukewalczak Oct 30, 2019
2d643c0
Update info for translators
lukewalczak Oct 30, 2019
f66a118
Merge branch 'rnmobile/spacer' into rnmobile/rangecell-styling
lukewalczak Oct 30, 2019
80104f1
Correct styles
lukewalczak Oct 30, 2019
93b7d58
Merge branch 'master' into rnmobile/rangecell-styling
lukewalczak Nov 5, 2019
26e7939
Refactor texts localizing
lukewalczak Nov 5, 2019
d321afd
merge master
jbinda Nov 6, 2019
b9c2b64
fix allowReset prop
jbinda Nov 7, 2019
cf85c25
Patch allowReset
lukewalczak Nov 7, 2019
500d414
a11y improvements
lukewalczak Nov 7, 2019
5d89738
Refactor custom button
lukewalczak Nov 8, 2019
7cd2f4f
Remove numberOfLines from cell label
lukewalczak Nov 8, 2019
ac07efa
Merge branch 'master' into rnmobile/rangecell-styling
lukewalczak Nov 15, 2019
135c4eb
Merge branch 'master' into rnmobile/rangecell-styling
lukewalczak Nov 15, 2019
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
.borderStyle {
border-bottom-width: 1px;
}

.isSelected {
border-bottom-width: 2px;
border-color: $blue-wordpress;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
.isSelected {
border-width: 2px;
border-color: $blue-wordpress;
}

.borderStyle {
border-width: 1px;
border-radius: 4px;
}
42 changes: 28 additions & 14 deletions packages/components/src/mobile/bottom-sheet/cell.native.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { TouchableOpacity, Text, View, TextInput, I18nManager, AccessibilityInfo } from 'react-native';
import { TouchableOpacity, Text, View, TextInput, I18nManager, Platform, AccessibilityInfo } from 'react-native';
import { isEmpty } from 'lodash';

/**
Expand Down Expand Up @@ -71,12 +71,15 @@ class BottomSheetCell extends Component {
leftAlign,
labelStyle = {},
valueStyle = {},
cellContainerStyle = {},
cellRowContainerStyle = {},
onChangeValue,
children,
editable = true,
separatorType,
style = {},
getStylesFromColorScheme,
allowReset,
...valueProps
} = this.props;

Expand All @@ -86,11 +89,14 @@ class BottomSheetCell extends Component {
const cellLabelCenteredStyle = getStylesFromColorScheme( styles.cellLabelCentered, styles.cellTextDark );
const cellLabelLeftAlignNoIconStyle = getStylesFromColorScheme( styles.cellLabelLeftAlignNoIcon, styles.cellTextDark );
const defaultMissingIconAndValue = leftAlign ? cellLabelLeftAlignNoIconStyle : cellLabelCenteredStyle;
const defaultLabelStyle = showValue || icon !== undefined ? cellLabelStyle : defaultMissingIconAndValue;
const defaultLabelStyle = showValue || icon !== undefined || allowReset ? cellLabelStyle : defaultMissingIconAndValue;

const drawSeparator = ( separatorType && separatorType !== 'none' ) || separatorStyle === undefined;
const drawTopSeparator = drawSeparator && separatorType === 'topFullWidth';

const cellContainerStyles = [ styles.cellContainer, cellContainerStyle ];
const rowContainerStyles = [ styles.cellRowContainer, cellRowContainerStyle ];

const onCellPress = () => {
if ( isValueEditable ) {
startEditing();
Expand Down Expand Up @@ -183,6 +189,8 @@ class BottomSheetCell extends Component {
};

const iconStyle = getStylesFromColorScheme( styles.icon, styles.iconDark );
const resetButtonStyle = getStylesFromColorScheme( styles.resetButton, styles.resetButtonDark );
const resetButtonText = Platform.OS === 'ios' ? __( 'Reset' ) : __( 'RESET' );
const containerPointerEvents = this.state.isScreenReaderEnabled && accessible ? 'none' : 'auto';

return (
Expand All @@ -196,22 +204,28 @@ class BottomSheetCell extends Component {
accessibilityHint
}
onPress={ onCellPress }
style={ { ...styles.clipToBounds, ...style } }
style={ [ styles.clipToBounds, style ] }
>
{ drawTopSeparator && (
<View style={ separatorStyle() } />
) }
<View style={ styles.cellContainer } pointerEvents={ containerPointerEvents }>
<View style={ styles.cellRowContainer }>
{ icon && (
<View style={ styles.cellRowContainer }>
<Dashicon icon={ icon } size={ 24 } color={ iconStyle.color } />
<View style={ platformStyles.labelIconSeparator } />
</View>
) }
<Text numberOfLines={ 1 } style={ [ defaultLabelStyle, labelStyle ] }>
{ label }
</Text>
<View style={ cellContainerStyles } pointerEvents={ containerPointerEvents }>
<View style={ rowContainerStyles }>
<View style={ styles.cellRowContainer }>
{ icon && (
<View style={ styles.cellRowContainer }>
<Dashicon icon={ icon } size={ 24 } color={ iconStyle.color } />
<View style={ platformStyles.labelIconSeparator } />
</View>
) }
<Text numberOfLines={ 1 } style={ [ defaultLabelStyle, labelStyle ] }>
Copy link
Member Author

@lukewalczak lukewalczak Nov 8, 2019

Choose a reason for hiding this comment

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

@etoledom If we remove numberOfLines={ 1 } we can have a not truncated label and it will look like:

wdyt?

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've also checked how it will affect other settings:

before after
Screenshot 2019-11-08 at 11 36 33 Screenshot 2019-11-08 at 11 36 28

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks better than cropping!
Let's go with it if it does't affect other instances of cells and controls.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've also checked how it will affect other settings

Again, better than cropping. Looks good to me :)

{ label }
</Text>
</View>
{ allowReset && <TouchableOpacity onPress={ allowReset } accessibilityRole={ 'button' }>
<Text style={ resetButtonStyle }>{ resetButtonText }
</Text>
</TouchableOpacity> }
jbinda marked this conversation as resolved.
Show resolved Hide resolved
</View>
{ showValue && getValueComponent() }
{ children }
Expand Down
75 changes: 45 additions & 30 deletions packages/components/src/mobile/bottom-sheet/range-cell.native.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
/**
* External dependencies
*/
import { Platform, AccessibilityInfo, findNodeHandle, TextInput, Slider } from 'react-native';
import { Platform, AccessibilityInfo, findNodeHandle, TextInput, Slider, View } from 'react-native';

/**
* WordPress dependencies
*/
import { _x, __, sprintf } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { withPreferredColorScheme } from '@wordpress/compose';

/**
* Internal dependencies
*/
import Cell from './cell';
import styles from './range-cell.scss';
import borderStyles from './borderStyles.scss';

class BottomSheetRangeCell extends Component {
constructor( props ) {
Expand Down Expand Up @@ -115,9 +117,11 @@ class BottomSheetRangeCell extends Component {
maximumValue = 10,
disabled,
step = 1,
minimumTrackTintColor = '#00669b',
preferredColorScheme,
minimumTrackTintColor = preferredColorScheme === 'light' ? '#00669b' : '#5198d9',
maximumTrackTintColor = Platform.OS === 'ios' ? '#e9eff3' : '#909090',
thumbTintColor = Platform.OS === 'android' && '#00669b',
getStylesFromColorScheme,
...cellProps
} = this.props;

Expand All @@ -130,49 +134,60 @@ class BottomSheetRangeCell extends Component {
cellProps.label, value
);

const defaultSliderStyle = getStylesFromColorScheme( styles.sliderTextInput, styles.sliderDarkTextInput );

return (
<Cell
{ ...cellProps }
cellContainerStyle={ styles.cellContainerStyles }
cellRowContainerStyle={ styles.cellRowStyles }
accessibilityRole={ 'none' }
editable={ false }
accessible={ accessible }
onPress={ this.onCellPress }
accessibilityLabel={ accessibilityLabel }
allowReset={ this.handleReset }
Copy link
Contributor

Choose a reason for hiding this comment

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

It's interesting that the root Cell component provides Reset option, but the reset functionality has to be handled by the parent component. It feels to me more like a generic button that could be anything. Maybe we can add it as an accessory view on this component, similar how we added the Switch?

Just by curiosity, I added allowReset to other controls, it shows the Reset button but it looks a bit off (apart than it does nothing). It doesn't feel to me that it deserves to be on the root Cell component, unless we plan to add reset to more controls? (In which case this might be a good-enough first step). cc @iamthomasbishop

IMG_3459

Choose a reason for hiding this comment

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

It doesn't feel to me that it deserves to be on the root Cell component, unless we plan to add reset to more controls? (In which case this might be a good-enough first step)

That's correct, I don't think there are any other planned components at the moment that will need an explicit reset button like this.

Choose a reason for hiding this comment

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

One more note: if we do end up adding reset to other cells/components, we'll want to display them in the same two-row way which provides more space for the reset button.

Copy link
Contributor

@jbinda jbinda Nov 6, 2019

Choose a reason for hiding this comment

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

Currently we don't have many settings in block but maybe it's not bad idea to allow reset single field (in case we will have much more settings). Correct me if I'm wrong but it seems now it's possible to manually delete/change the value or clear all settings.

Copy link
Member Author

@lukewalczak lukewalczak Nov 7, 2019

Choose a reason for hiding this comment

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

  • Reset button is placed within a Cell component, because we want to locate it next to the label (Height in pixels) which is defined there actually, otherwise it has to be positioned absolutely.

accessibilityHint={
/* translators: accessibility text (hint for focusing a slider) */
__( 'Double tap to change the value using slider' )
}
>
<Slider
value={ this.validateInput( sliderValue ) }
defaultValue={ defaultValue }
disabled={ disabled }
step={ step }
minimumValue={ minimumValue }
maximumValue={ maximumValue }
minimumTrackTintColor={ minimumTrackTintColor }
maximumTrackTintColor={ maximumTrackTintColor }
thumbTintColor={ thumbTintColor }
onValueChange={ this.handleChange }
onSlidingComplete={ this.handleValueSave }
ref={ ( slider ) => {
this.sliderRef = slider;
} }
style={ styles.slider }
accessibilityRole={ 'adjustable' }
/>
<TextInput
style={ [ styles.sliderTextInput, hasFocus ? styles.isSelected : {} ] }
onChangeText={ this.handleChange }
onFocus={ this.handleToggleFocus }
onBlur={ this.handleToggleFocus }
keyboardType="number-pad"
returnKeyType="done"
value={ `${ sliderValue }` }
/>
<View style={ styles.container }>
<Slider
value={ this.validateInput( sliderValue ) }
defaultValue={ defaultValue }
disabled={ disabled }
step={ step }
minimumValue={ minimumValue }
maximumValue={ maximumValue }
minimumTrackTintColor={ minimumTrackTintColor }
maximumTrackTintColor={ maximumTrackTintColor }
thumbTintColor={ thumbTintColor }
onValueChange={ this.handleChange }
onSlidingComplete={ this.handleValueSave }
ref={ ( slider ) => {
this.sliderRef = slider;
} }
style={ styles.slider }
accessibilityRole={ 'adjustable' }
/>
<TextInput
style={ [
defaultSliderStyle,
borderStyles.borderStyle,
hasFocus && borderStyles.isSelected,
] }
onChangeText={ this.handleChange }
onFocus={ this.handleToggleFocus }
onBlur={ this.handleToggleFocus }
keyboardType="number-pad"
returnKeyType="done"
value={ `${ sliderValue }` }
/>
</View>
</Cell>
);
}
}

export default BottomSheetRangeCell;
export default withPreferredColorScheme( BottomSheetRangeCell );
27 changes: 24 additions & 3 deletions packages/components/src/mobile/bottom-sheet/range-cell.native.scss
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,35 @@
height: 25px;
align-self: center;
margin-left: 10px;
border-width: 1px;
border-radius: 4px;
border-color: $dark-gray-150;
border-color: $light-gray-400;
padding-top: 0;
padding-bottom: 0;
text-align: center;
}

.sliderDarkTextInput {
border-color: $gray-70;
}

.isSelected {
border-width: 2px;
border-color: $blue-wordpress;
}

.container {
flex-direction: row;
align-items: center;
min-height: 48px;
}

.cellContainerStyles {
flex-direction: column;
align-items: flex-start;
}

.cellRowStyles {
min-height: 48px;
margin-bottom: -13px;
width: 100%;
justify-content: space-between;
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,15 @@
padding: 5px;
}

.resetButton {
font-size: 17px;
color: $blue-wordpress;
}

.resetButtonDark {
color: $blue-30;
}

// Cell

.cellContainer {
Expand Down