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

Leaner code for the native mobile Toolbar #9064

Merged
merged 6 commits into from
Aug 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 25 additions & 0 deletions packages/components/src/button/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* External dependencies
*/
import { TouchableOpacity } from 'react-native';

/**
* WordPress dependencies
*/
import { forwardRef } from '@wordpress/element';

export function Button( props ) {
const { onClick } = props;
const ariaLabel = props[ 'aria-label' ];
return (
<TouchableOpacity
accessible={ true }
accessibilityLabel={ ariaLabel }
onPress={ onClick }
>
{ props.children }
</TouchableOpacity>
);
}

export default forwardRef( Button );
17 changes: 13 additions & 4 deletions packages/components/src/dashicon/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ OR if you're looking to change now SVGs get output, you'll need to edit strings
*/
import { Component } from '@wordpress/element';
import SVG from './svg';
import Path from './path';

export default class Dashicon extends Component {
shouldComponentUpdate( nextProps ) {
Expand Down Expand Up @@ -886,10 +887,18 @@ export default class Dashicon extends Component {
const iconClass = [ 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' );

return (
<SVG className={ iconClass }
size={ size }
path= { path }
/>
<SVG
aria-hidden
role="img"
focusable="false"
className={ iconClass }
xmlns="http://www.w3.org/2000/svg"
width={ size }
height={ size }
viewBox="0 0 20 20"
>
<Path d={ path } />
Copy link
Contributor

@SergioEstevao SergioEstevao Aug 17, 2018

Choose a reason for hiding this comment

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

I'm not sure this will work on the web. If you see my original code on my PR for the web the Path element is defined using lowercase. i.e: path

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you did know you redefined Path also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I extracted the Path component to its own component and Path is imported here https://github.com/WordPress/gutenberg/pull/9064/files#diff-297ebddffcddddf088a365bab7f0909cR13. That should do it, although I think there's probably a problem with the way I've defined the web side variants (related to the comment).

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 component is meant to be modified in the Dashicons repository first.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the comments I saw on the file I thought this was the case, if we could a pointer to where to make the changes it will be great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dashicons are edited in https://github.com/WordPress/dashicons. Feel free to ping me for help, I can fasttrack any changes you'd like.

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 think this might be the file to modify on the Dashicons repo: https://github.com/WordPress/dashicons/blob/master/sources/react/index-footer.jsx. Not sure yet how to integrate that repo/project and have it hot-reloading in the React Native app but yeah, let's see.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you'd like to make a PR, I can approve it for you and ship it to Gutenberg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@youknowriad , it would still be good to review the proposal in this PR and agree on whether we like/want it or not before migrating the change to the Dashicons repo, so we take advantage of the quick turnaround time we enjoy with the Gutenberg+ReactNative repo combo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the best approach here would be to have an alternative index.native.js for the whole dashicon component instead of extracting Path and Svg

</SVG>
);
}
}
1 change: 1 addition & 0 deletions packages/components/src/dashicon/path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default ( props ) => ( <path { ...props } /> );
3 changes: 3 additions & 0 deletions packages/components/src/dashicon/path.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { Path } from 'react-native-svg';

export default ( props ) => ( <Path { ...props } /> );
22 changes: 5 additions & 17 deletions packages/components/src/dashicon/svg.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,5 @@
export default function SVG( props, ref ) {
const { size, path, iconClass } = props
return (
<svg
aria-hidden
role="img"
focusable="false"
className={ iconClass }
xmlns="http://www.w3.org/2000/svg"
width={ size }
height={ size }
viewBox="0 0 20 20"
>
<path d={ path } />
</svg>
);
}
export default ( props ) => (
<svg { ...props } >
{ props.children }
</svg>
);
15 changes: 6 additions & 9 deletions packages/components/src/dashicon/svg.native.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import Svg,{ Path } from 'react-native-svg';
import Svg from 'react-native-svg';

export default function SVG( props, ref ) {
const { size, path, iconClass } = props
return (
<Svg height={ size } width={ size }>
<Path d={ path } />
</Svg>
);
}
export default ( props ) => (
<Svg width={ props.width } height={ props.height } >
{ props.children }
</Svg>
);
41 changes: 0 additions & 41 deletions packages/components/src/icon-button/index.native.js

This file was deleted.

2 changes: 1 addition & 1 deletion packages/components/src/index.native.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Components
// eslint-disable-next-line camelcase
export { default as Dashicon } from './dashicon';
export { default as Toolbar } from './toolbar';
export { default as Toolbar } from './toolbar';
11 changes: 7 additions & 4 deletions packages/components/src/toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { flatMap } from 'lodash';
* Internal dependencies
*/
import IconButton from '../icon-button';
import ToolbarContainer from './toolbar-container';
import ToolbarButtonContainer from './toolbar-button-container';

/**
* Renders a toolbar with controls.
Expand Down Expand Up @@ -54,11 +56,12 @@ function Toolbar( { controls = [], children, className } ) {
}

return (
<div className={ classnames( 'components-toolbar', className ) }>
<ToolbarContainer className={ classnames( 'components-toolbar', className ) }>
{ flatMap( controlSets, ( controlSet, indexOfSet ) => (
controlSet.map( ( control, indexOfControl ) => (
<div
<ToolbarButtonContainer
key={ [ indexOfSet, indexOfControl ].join() }
keyProp={ [ indexOfSet, indexOfControl ].join() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we repeat key and keyProp?

Copy link
Contributor Author

@hypest hypest Aug 17, 2018

Choose a reason for hiding this comment

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

Ah, good catch, I should have mentioned this "gotcha" in the PR description. So, React has this set of "special" props that it doesn't propagate down to the child components. They call them Special Props. In this case, the key property cannot be picked up by the ToolbarButtonContainer component and we need to replicate it on purpose into a differently named prop, keyProp.

I'll update the PR description to include it for future ref.

className={ indexOfSet > 0 && indexOfControl === 0 ? 'has-left-divider' : null }
>
<IconButton
Expand All @@ -77,11 +80,11 @@ function Toolbar( { controls = [], children, className } ) {
disabled={ control.isDisabled }
/>
{ control.children }
</div>
</ToolbarButtonContainer>
) )
) ) }
{ children }
</div>
</ToolbarContainer>
);
}

Expand Down
89 changes: 0 additions & 89 deletions packages/components/src/toolbar/index.native.js

This file was deleted.

8 changes: 8 additions & 0 deletions packages/components/src/toolbar/toolbar-button-container.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default ( props ) => (
<div
key={ props.keyProp }
className={ props.className }
>
{ props.children }
</div>
);
13 changes: 13 additions & 0 deletions packages/components/src/toolbar/toolbar-button-container.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* External dependencies
*/
import { View } from 'react-native';

export default ( props ) => (
<View
key={ props.keyProp }
style={ props.className }
>
{ props.children }
</View>
);
10 changes: 10 additions & 0 deletions packages/components/src/toolbar/toolbar-container.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* External dependencies
*/
import classnames from 'classnames';

export default ( props ) => (
<div className={ classnames( 'components-toolbar', props.className ) }>
{ props.children }
</div>
);
10 changes: 10 additions & 0 deletions packages/components/src/toolbar/toolbar-container.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* External dependencies
*/
import { View } from 'react-native';

export default ( props ) => (
<View style={ { flex: 1, flexDirection: 'row' } }>
{ props.children }
</View>
);
4 changes: 4 additions & 0 deletions packages/components/src/tooltip/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import React from 'react';

// for the native mobile, just shortcircuit the Tooltip to return its child
export default ( props ) => React.Children.only( props.children );
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
*/
import { __ } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import {
IconButton,
import {
Toolbar,
} from '@wordpress/components';
import { ESCAPE, LEFT, RIGHT, UP, DOWN, BACKSPACE, ENTER, displayShortcut } from '@wordpress/keycodes';
Expand All @@ -15,9 +14,7 @@ import { prependHTTP } from '@wordpress/url';
*/
//import PositionedAtSelection from './positioned-at-selection';
//import URLInput from '../../url-input';
import { filterURLForDisplay } from '../../../utils/url';

import { Text } from 'react-native'
//import { filterURLForDisplay } from '../../../utils/url';

const FORMATTING_CONTROLS = [
{
Expand Down Expand Up @@ -173,9 +170,9 @@ class FormatToolbar extends Component {
}

render() {
const { formats, enabledControls = DEFAULT_CONTROLS, customControls = [], selectedNodeId } = this.props;
const { linkValue, settingsVisible, opensInNewWindow } = this.state;
const isAddingLink = formats.link && formats.link.isAdding;
const { enabledControls = DEFAULT_CONTROLS, customControls = [] } = this.props;
// const { linkValue, settingsVisible, opensInNewWindow } = this.state;
// const isAddingLink = formats.link && formats.link.isAdding;

const toolbarControls = FORMATTING_CONTROLS.concat( customControls )
.filter( ( control ) => enabledControls.indexOf( control.format ) !== -1 )
Expand All @@ -197,10 +194,10 @@ class FormatToolbar extends Component {
onClick: this.toggleFormat( control.format ),
isActive: this.isFormatActive( control.format ),
};
} );
} );

return (
<Toolbar controls={ toolbarControls } />
return (
<Toolbar controls={ toolbarControls } />
);
}
}
Expand Down
Loading