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/native toolbar component ui #11827

Merged
merged 39 commits into from
Nov 22, 2018
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
ff3cd91
Change dashicon color
marecar3 Nov 8, 2018
e28480f
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 13, 2018
3cff075
Fixed Toolbar style
marecar3 Nov 13, 2018
6a06306
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 13, 2018
442a6eb
Style refactor
marecar3 Nov 13, 2018
5c6ae4f
Small refinment
marecar3 Nov 13, 2018
b60acce
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 13, 2018
565bd08
Small fixes
marecar3 Nov 13, 2018
125a576
Small fixes
marecar3 Nov 13, 2018
7588291
Small fix
marecar3 Nov 13, 2018
826bec4
Temp push
marecar3 Nov 19, 2018
c79a483
Implemented selected state
marecar3 Nov 19, 2018
a5aa2f2
Fixed naming
marecar3 Nov 19, 2018
7c6c0f4
Small fixes
marecar3 Nov 19, 2018
4aaac80
Rename css files
marecar3 Nov 19, 2018
54440b6
Lint fixes
marecar3 Nov 19, 2018
ee7a909
Added EOF line
marecar3 Nov 19, 2018
2560918
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 19, 2018
2fa504d
Removed path const
marecar3 Nov 20, 2018
66e210f
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 20, 2018
45643fa
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 20, 2018
4fed682
Small fix
marecar3 Nov 20, 2018
dc8c21a
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 20, 2018
0809e9f
Small fix
marecar3 Nov 20, 2018
fc6cf65
Fixed lint errors
marecar3 Nov 20, 2018
bdb66bf
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 20, 2018
1c7dfc4
Fixed styles from comment
marecar3 Nov 20, 2018
0162da0
Fixed path
marecar3 Nov 21, 2018
51956f8
Flow error fix
marecar3 Nov 21, 2018
746c90a
Fixes from comments
marecar3 Nov 21, 2018
49e82b4
FIxed lint errors
marecar3 Nov 21, 2018
b22ad2e
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 21, 2018
56640cd
Fixes from comments
marecar3 Nov 21, 2018
591ce94
Updated dashicon unselected state color
marecar3 Nov 22, 2018
0c81045
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 22, 2018
5e6e9d2
Fixes from comments
marecar3 Nov 22, 2018
bfdab84
Fixed lint errors
marecar3 Nov 22, 2018
a9aa8e5
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 22, 2018
168f08b
Merge branch 'master' into rnmobile/native-toolbar-component-ui
marecar3 Nov 22, 2018
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
4 changes: 2 additions & 2 deletions packages/block-library/src/image/edit.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import RNReactNativeGutenbergBridge from 'react-native-gutenberg-bridge';
* Internal dependencies
*/
import { MediaPlaceholder, RichText, BlockControls } from '@wordpress/editor';
import { Toolbar, IconButton } from '@wordpress/components';
import { Toolbar, ToolbarButton } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

export default function ImageEdit( props ) {
Expand Down Expand Up @@ -40,7 +40,7 @@ export default function ImageEdit( props ) {

const toolbarEditButton = (
<Toolbar>
<IconButton
<ToolbarButton
className="components-toolbar__control"
label={ __( 'Edit image' ) }
icon="edit"
Expand Down
44 changes: 40 additions & 4 deletions packages/components/src/button/index.native.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,56 @@
/**
* External dependencies
*/
import { TouchableOpacity, Text, View } from 'react-native';
import { StyleSheet, TouchableOpacity, Text, View } from 'react-native';

const styles = StyleSheet.create( {
container: {
flex: 1,
padding: 3,
justifyContent: 'center',
alignItems: 'center',
},
buttonInactive: {
flex: 1,
flexDirection: 'row',
justifyContent: 'center',
alignItems: 'center',
aspectRatio: 1,
backgroundColor: 'white',
},
buttonActive: {
flex: 1,
flexDirection: 'row',
justifyContent: 'center',
alignItems: 'center',
borderRadius: 6,
borderColor: '#2e4453',
aspectRatio: 1,
backgroundColor: '#2e4453',
},
subscriptInactive: {
fontVariant: [ 'small-caps' ],
color: '#87a6bc',
},
subscriptActive: {
fontVariant: [ 'small-caps' ],
color: 'white',
},
} );

export default function Button( props ) {
const { children, onClick, 'aria-label': ariaLabel, 'aria-pressed': ariaPressed, 'data-subscript': subscript } = props;

return (
<TouchableOpacity
accessible={ true }
accessibilityLabel={ ariaLabel }
onPress={ onClick }
style={ { borderColor: ariaPressed ? 'black' : 'white', borderWidth: 1, borderRadius: 2 } }
style={ styles.container }
>
<View style={ { height: 44, width: 44, flexDirection: 'row', justifyContent: 'center', alignItems: 'center' } }>
<View style={ ariaPressed ? styles.buttonActive : styles.buttonInactive }>
{ children }
{ subscript && ( <Text style={ { fontVariant: [ 'small-caps' ] } }>{ subscript }</Text> ) }
{ subscript && ( <Text style={ ariaPressed ? styles.subscriptActive : styles.subscriptInactive }>{ subscript }</Text> ) }
</View>
</TouchableOpacity>
);
Expand Down
4 changes: 4 additions & 0 deletions packages/components/src/dashicon/icon-class.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export const IconClass = ( props ) => {
const { icon, className } = props;
return [ 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' );
};
4 changes: 4 additions & 0 deletions packages/components/src/dashicon/icon-class.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export const IconClass = ( props ) => {
const { icon, className, ariaPressed } = props;
return [ ariaPressed ? 'dashicon-active' : 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' );
};
9 changes: 6 additions & 3 deletions packages/components/src/dashicon/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,21 @@ import { Component } from '@wordpress/element';
* Internal dependencies
*/
import { Path, SVG } from '../primitives';
import { IconClass } from './icon-class';

export default class Dashicon extends Component {
shouldComponentUpdate( nextProps ) {

return (
this.props.icon !== nextProps.icon ||
this.props.size !== nextProps.size ||
this.props.className !== nextProps.className
this.props.className !== nextProps.className ||
this.props.ariaPressed !== nextProps.ariaPressed
);
}

render() {
const { icon, className, size = 20 } = this.props;
const { icon, size = 20 } = this.props;
let path;

switch ( icon ) {
Expand Down Expand Up @@ -896,7 +899,7 @@ export default class Dashicon extends Component {
return null;
}

const iconClass = [ 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' );
const iconClass = IconClass(this.props);

return (
<SVG
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/icon-button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import Dashicon from '../dashicon';
class IconButton extends Component {
render() {
const { icon, children, label, className, tooltip, shortcut, ...additionalProps } = this.props;
const { 'aria-pressed': ariaPressed } = this.props;
const classes = classnames( 'components-icon-button', className );
const tooltipText = tooltip || label;

Expand All @@ -42,7 +43,7 @@ class IconButton extends Component {

let element = (
<Button aria-label={ label } { ...additionalProps } className={ classes }>
{ isString( icon ) ? <Dashicon icon={ icon } /> : icon }
{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } /> : icon }
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand this change? why do we need aria-pressed here since we're applying it to the parent button?

Copy link
Contributor

Choose a reason for hiding this comment

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

The underlying issue is that we need to find a way to set the color of a Dashicon. Since the web has CSS it can use class names, inheritance, and fill: currentColor to get to that. But we don't have that on native.

Ideally we'd be able to set color or style on a Dashicon, I took an alternate approach in #12403, but that was easier since I was using Dashicon directly. For this we'd have to make everything in the hierarchy forward the style down to Dashicon. As mentioned on that PR, you can set extraProps in IconButton, but those will go to the Button, not the icon.

We just found something that worked to keep us moving although we were aware it wasn't great, so I'm happy to discuss a better solution 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned about these small tweaks having an impact on the web code base, can't we create an icon-button native version instead?

Copy link
Member

@aduth aduth Mar 1, 2019

Choose a reason for hiding this comment

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

The underlying issue is that we need to find a way to set the color of a Dashicon. Since the web has CSS it can use class names, inheritance, and fill: currentColor to get to that. But we don't have that on native.

@koke Can you point me to where this occurs? As best I can follow, the main impact of the ariaPressed prop is in changing the class behavior of Dashicon to apply dashicon-active in the native context, but I see no reference to dashicon-active anywhere in Gutenberg or Gutenberg Mobile aside from where we apply it.

I'd tend to agree with @youknowriad here. "Pressed" does not resound to me as a characteristic of an icon, and thus it does not make sense as a prop. It does, however, seem natural to describe the state of an IconButton.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is what's setting the style https://github.com/WordPress/gutenberg/blob/master/packages/components/src/primitives/svg/style.native.scss#L6

I agree that this isn't ideal. It seems like the introduction of iconProps in #13977 could be a good solution to pass custom styles from IconButton to Dashicon?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know that #13977 is an improvement here, as I expect you'd continue to treat it as a pressed icon, which is the root of the issue ("pressed" is not a sensible state of an icon).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's right.
It looks like the right logic here would be that IconButton set different style props on Dashicon depending on isActive. This seems to be what the web is doing, although via CSS.
However this is overriden by CSS in the ToolbarButton component.

We keep bumping into this on native: the web can rely on nested CSS definition to pass style down to lower level components, but we can't do that unless we pass them explicitly. I still think ToolbarButton should be somehow deciding what the style for Dashicon is when normal/pressed, which is why now having iconProps in IconButton seems like a way out.

@etoledom can you handle this (removing ariaPressed from Dashicon) as part of #13977?

{ children }
</Button>
);
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
export * from './primitives';
export { default as Dashicon } from './dashicon';
export { default as Toolbar } from './toolbar';
export { default as ToolbarButton } from './toolbar-button';
export { default as withSpokenMessages } from './higher-order/with-spoken-messages';
export { default as IconButton } from './icon-button';
export { createSlotFill, Slot, Fill, Provider as SlotFillProvider } from './slot-fill';
Expand Down
24 changes: 11 additions & 13 deletions packages/components/src/primitives/svg/index.native.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
/**
* External dependencies
*/
import { omit } from 'lodash';
import { Svg } from 'react-native-svg';

/**
* Internal dependencies
*/
import styles from '../../dashicon/style.scss';
import styles from './style.scss';

export {
Circle,
Expand All @@ -21,24 +20,23 @@ export const SVG = ( props ) => {
// We're using the react-native-classname-to-style plugin, so when a `className` prop is passed it gets converted to `style` here.
// Given it carries a string (as it was originally className) but an object is expected for `style`,
// we need to check whether `style` exists and is a string, and convert it to an object
let styleKeys = new Array();
const styleValues = new Array();
if ( typeof props.style === 'string' || props.style instanceof String ) {
styleKeys = props.style.split( ' ' );
styleKeys.forEach( ( element ) => {
const oneStyle = styles[ element ];
if ( oneStyle !== undefined ) {
styleValues.push( oneStyle );
}
} );

let styleValues = {};
if ( typeof props.style === 'string' ) {
const oneStyle = props.style.split( ' ' ).map( element => styles[ element ] ).filter( Boolean );
styleValues = Object.assign( styleValues, ...oneStyle );
}

const safeProps = styleValues.length === 0 ? { ...omit( props, [ 'style' ] ) } : { ...props, style: styleValues };
const safeProps = { ...props, style: styleValues };

return (
<Svg
//We want to re-render when style color is changed
key={ safeProps.style.color }
height="100%"
width="100%"
{ ...safeProps }
/>
);
};

14 changes: 14 additions & 0 deletions packages/components/src/primitives/svg/style.native.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
.dashicon {
color: #87a6bc;
fill: currentColor;
}

.dashicon-active {
color: #fff;
fill: currentColor;
}

.dashicons-insert {
color: #87a6bc;
fill: currentColor;
}
7 changes: 7 additions & 0 deletions packages/components/src/toolbar/style.native.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.container {
flex-direction: row;
border-right-width: 1px;
border-right-color: #e9eff3;
padding-left: 5px;
padding-right: 5px;
}
8 changes: 6 additions & 2 deletions packages/components/src/toolbar/toolbar-container.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@
*/
import { View } from 'react-native';

export default ( props ) => (
<View style={ { flexDirection: 'row' } }>
import styles from './style.native.scss';

const ToolbarContainer = ( props ) => (
<View style={ styles.container }>
{ props.children }
</View>
);

export default ToolbarContainer;
1 change: 1 addition & 0 deletions packages/html-entities/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"description": "HTML entity utilities for WordPress.",
"author": "The WordPress Contributors",
"license": "GPL-2.0-or-later",
"react-native": "src/index",
"keywords": [
"wordpress",
"html",
Expand Down