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.

Sign up for GitHub

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] iOS DarkMode toolbar buttons #17356

Merged
merged 6 commits into from
Oct 3, 2019
Merged

[RNMobile] iOS DarkMode toolbar buttons #17356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3823c6c
Send theme and active props to button children
etoledom Sep 6, 2019
fec60d5
Remove ariaPressed prop from icon
etoledom Sep 6, 2019
d9d6d9e
DarkMode color improvement on toolbar separator
etoledom Sep 6, 2019
e03c6f7
Updated using new DarkMode refactor
etoledom Sep 26, 2019
640b17f
Improved names. Avoiding mentioning "theme" to refer to color schemes.
etoledom Sep 26, 2019
f4e5629
Merge branch 'master' into rnmobile/darkmode-toolbar-buttons
etoledom Sep 27, 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
27 changes: 23 additions & 4 deletions packages/components/src/button/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@
* External dependencies
*/
import { StyleSheet, TouchableOpacity, Text, View, Platform } from 'react-native';
import { compact } from 'lodash';

/**
* WordPress dependencies
*/
import { Children, cloneElement } from '@wordpress/element';
import { withPreferredColorScheme } from '@wordpress/compose';

const isAndroid = Platform.OS === 'android';
const marginBottom = isAndroid ? -0.5 : 0;
Expand Down Expand Up @@ -40,6 +47,9 @@ const styles = StyleSheet.create( {
marginLeft,
marginBottom,
},
subscriptInactiveDark: {
color: '#a7aaad', // $gray_20
},
subscriptActive: {
color: 'white',
fontWeight: 'bold',
Expand All @@ -50,13 +60,14 @@ const styles = StyleSheet.create( {
},
} );

export default function Button( props ) {
export function Button( props ) {
const {
children,
onClick,
disabled,
hint,
fixedRatio = true,
getStylesFromColorScheme,
'aria-disabled': ariaDisabled,
'aria-label': ariaLabel,
'aria-pressed': ariaPressed,
Expand All @@ -66,7 +77,7 @@ export default function Button( props ) {
const isDisabled = ariaDisabled || disabled;

const buttonViewStyle = {
opacity: isDisabled ? 0.2 : 1,
opacity: isDisabled ? 0.3 : 1,
...( fixedRatio && styles.fixedRatio ),
...( ariaPressed ? styles.buttonActive : styles.buttonInactive ),
};
Expand All @@ -80,6 +91,12 @@ export default function Button( props ) {
states.push( 'disabled' );
}

const subscriptInactive = getStylesFromColorScheme( styles.subscriptInactive, styles.subscriptInactiveDark );

const newChildren = Children.map( compact( children ), ( child ) => {
Copy link
Contributor

@Tug Tug Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rather we use a context for this:

// in Button:
export const ButtonContext = React.createContext( {
  active: false,
  colorScheme: 'light',
} );

export function Button( props ) {

	...

	const buttonContext = {
	  active: props.active,
	  colorScheme: props.colorScheme,
	};

	{ Children.count( children ) &&
		<ButtonContext.Provider value={ buttonContext }>
			{ children }
		</ButtonContext.Provider>
	}

	...

// in IconButton
<ButtonContext.Consumer>
    { ( { colorScheme, active } )  =>
    	<Icon icon={ icon } ariaPressed={ active } colorScheme={ colorScheme } />
</ButtonContext.Consumer>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the idea from here: https://github.com/WordPress/gutenberg/blob/82a96dd640354ba5068dee5a10e6195e96ea65bb/packages/components/src/primitives/block-quotation/index.native.js#L15

And the main idea was to clear up IconButton from sending the ariaPressed or active related states.

I like that we can get enough information to style any child of button, but you are right that this might not be the best option.

I'll give this a try!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about it, I guess using cloneElement should not have any impact on performance, it's just a bit more limited in terms of features. Basically, you'll only be able to access those props from the direct children of the button, whereas a consumer could be added anywhere. If direct children is fine then I guess we can keep it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the main (and only, so far) usage of this is the IconButton where the Icon component is a direct child.
So, if you think it's fine to keep this, let's move on 👍

I restarted a CI job to get the ✅ from travis.
Thanks for your review!

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know about React Context before. Thanks for leaving this comment @Tug !

return cloneElement( child, { colorScheme: props.preferredColorScheme, active: ariaPressed } );
} );

return (
<TouchableOpacity
activeOpacity={ 0.7 }
Expand All @@ -94,10 +111,12 @@ export default function Button( props ) {
>
<View style={ buttonViewStyle }>
<View style={ { flexDirection: 'row' } }>
{ children }
{ subscript && ( <Text style={ ariaPressed ? styles.subscriptActive : styles.subscriptInactive }>{ subscript }</Text> ) }
{ newChildren }
{ subscript && ( <Text style={ ariaPressed ? styles.subscriptActive : subscriptInactive }>{ subscript }</Text> ) }
</View>
</View>
</TouchableOpacity>
);
}

export default withPreferredColorScheme( Button );
3 changes: 1 addition & 2 deletions packages/components/src/icon-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ function IconButton( props, ref ) {
labelPosition,
...additionalProps
} = props;
const { 'aria-pressed': ariaPressed } = additionalProps;
const classes = classnames( 'components-icon-button', className, {
'has-text': children,
} );
Expand Down Expand Up @@ -56,7 +55,7 @@ function IconButton( props, ref ) {
className={ classes }
ref={ ref }
>
<Icon icon={ icon } ariaPressed={ ariaPressed } />
<Icon icon={ icon } />
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that it's safe to stop passing down this prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's basically being replaced by this line here:
https://github.com/WordPress/gutenberg/blob/82a96dd640354ba5068dee5a10e6195e96ea65bb/packages/components/src/button/index.native.js#L97
Were we send the colorScheme and the active state to all children of Button.

But considering @Tug's comment here: https://github.com/WordPress/gutenberg/pull/17356/files#r328582023 I might need to change this anyway.

What do you think?

Copy link
Member

@gziolo gziolo Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I checked the history and it looks like ariaPressed was introduced in #11827. It was never really used in the web code. Should it be also removed in other places?

In particular, I think about Dashicon implementation which used to pass this prop down but it seems to longer be necessary. See:

gutenberg/packages/components/src/dashicon/index.js

Line 895 in eb3a82f

const iconClass = getIconClassName( icon, className, ariaPressed );

Copy link
Member

Choose a reason for hiding this comment

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

Filed here: #17741.

{ children }
</Button>
);
Expand Down
5 changes: 3 additions & 2 deletions packages/components/src/primitives/svg/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ export {
} from 'react-native-svg';

export const SVG = ( props ) => {
const colorScheme = props.colorScheme || 'light';
const stylesFromClasses = ( props.className || '' ).split( ' ' ).map( ( element ) => styles[ element ] ).filter( Boolean );
const stylesFromAriaPressed = props.ariaPressed ? styles[ 'is-active' ] : styles[ 'components-toolbar__control' ];
const styleValues = Object.assign( {}, props.style, stylesFromAriaPressed, ...stylesFromClasses );
const defaultStyle = props.active ? styles[ 'is-active' ] : styles[ 'components-toolbar__control-' + colorScheme ];
const styleValues = Object.assign( {}, props.style, defaultStyle, ...stylesFromClasses );

const safeProps = { ...props, style: styleValues };

Expand Down
10 changes: 8 additions & 2 deletions packages/components/src/primitives/svg/style.native.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
.dashicon,
.components-toolbar__control {
.dashicon-light,
.components-toolbar__control-light {
color: #7b9ab1;
fill: currentColor;
}

.dashicon-dark,
.components-toolbar__control-dark {
color: $gray_20;
fill: currentColor;
}

.dashicon-active,
.is-active {
color: #fff;
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/toolbar/style.native.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
}

.containerDark {
border-color: #515459;
border-color: #525354;
}