-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Changes from 5 commits
12c6f5a
9b306ee
599774d
e961d17
6f6f418
aed7db6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default ( props ) => ( <path { ...props } /> ); |
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 } /> ); |
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> | ||
); |
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> | ||
); |
This file was deleted.
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'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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() } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we repeat key and keyProp? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I'll update the PR description to include it for future ref. |
||
className={ indexOfSet > 0 && indexOfControl === 0 ? 'has-left-divider' : null } | ||
> | ||
<IconButton | ||
|
@@ -77,11 +80,11 @@ function Toolbar( { controls = [], children, className } ) { | |
disabled={ control.isDisabled } | ||
/> | ||
{ control.children } | ||
</div> | ||
</ToolbarButtonContainer> | ||
) ) | ||
) ) } | ||
{ children } | ||
</div> | ||
</ToolbarContainer> | ||
); | ||
} | ||
|
||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export default ( props ) => ( | ||
<div | ||
key={ props.keykeyProp } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding of the docs is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but keykeyPro? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see now, that's a typo, good catch! Fixed with aed7db6. |
||
className={ props.className } | ||
> | ||
{ props.children } | ||
</div> | ||
); |
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> | ||
); |
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> | ||
); |
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> | ||
); |
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 ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 andPath
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).There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 extractingPath
andSvg