Implement toolbar on React Native #9012

SergioEstevao · 2018-08-15T10:56:08Z

Description

This PR adds support for the Toolbar component on React Native.

At the moment it only renders the toolbar for the RichText component.

How has this been tested?

Using this PR on the gutenberg mobile repo.

You can see that for each RichText block we get the basic format interface.

Screenshots

Types of changes

This adds RN version of the following components:

Toolbar
Rich Text / Format-Toolbar
DashIcon
IconButton

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

* Dedicated components for SVG, Path * Fix lint errors * Dedicated native components for button, icon-button, tooltip * Dedicated native mobile comps for Toolbar containers * Small code styling fix * Fix double "key" typo in prop name

hypest · 2018-08-17T11:10:39Z

~~Hmm, the conflicts with the snapshot files surface the need to componentize and nativize yet another component, the <g> element.~~

Updated to latest master and after resolving conflicts it seems there is no need for nativizing <g>.

gziolo

Should we group all primitives in one folder inside components package? At the moment we have:

View
Button
SVG
Path

I'm sure we will have more components like that:

Text
Link
Image
...

gziolo · 2018-08-20T12:17:38Z

packages/components/src/button/index.native.js

+import { forwardRef } from '@wordpress/element';
+
+export function Button( props ) {
+ const { onClick } = props;


You can also use deconstructing with aria-label:

const { children, onClick, 'aria-label': ariaLabel } = props;

and include children props, too.

gziolo · 2018-08-20T12:22:50Z

packages/components/src/index.native.js

@@ -0,0 +1,4 @@
+// Components
+// eslint-disable-next-line camelcase


Why do we need this line: eslint-disable-next-line camelcase?

gziolo · 2018-08-20T12:25:24Z

packages/components/src/toolbar/toolbar-button-container.js

@@ -0,0 +1,9 @@
+const ToolbarButtonContainer = ( props ) => (
+ <div
+ key={ props.keyProp }


You don't need to pass it down, it is used only by React for its internal optimizations. See related React doc: https://reactjs.org/docs/lists-and-keys.html#extracting-components-with-keys.

gziolo · 2018-08-20T12:27:36Z

packages/components/src/tooltip/index.native.js

+import React from 'react';
+
+// for the native mobile, just shortcircuit the Tooltip to return its child
+export default ( props ) => React.Children.only( props.children );


This doesn't look like a tooltip implementation at all :)

@hypest did this changes I think it's just a stub implementation to avoid changes on the format-toolbar component that uses this. To be honest tooltips don't make a lot of sense on native apps. You are not hovering any elements on them...

gziolo · 2018-08-20T12:30:54Z

packages/keycodes/src/platform.js

+ *
+ * @return {boolean} True if MacOS; false otherwise.
+ */
+export function isAppleOS( _window = window ) {


Can we keep the original name?

We can re-export it instead of aliasing:

export const isMacOS = isAppleOS;` // vs export * from './platform';

I'm a bit confused, do we to keep the function name isAppleOS and only add an extra export with the name isMacOS?

I'm not sure why we needed isAppleOS in the first place. If we can remove it then sure.

gziolo · 2018-08-20T12:32:28Z

packages/keycodes/src/platform.native.js

+ * @return {boolean} True if iOS; false otherwise.
+ */
+// eslint-disable-next-line no-unused-vars
+export function isAppleOS( _window = window ) {


To make it even simpler you can do the following:

FIle name: platform.ios.js:

export const isMacOs = () => true;

FIle name: platform.android.js:

export const isMacOs = () => false;

gziolo · 2018-08-20T12:35:17Z

packages/editor/src/components/rich-text/index.native.js

@@ -69,25 +84,68 @@ export class RichText extends Component {
 return true;
 }

+ isFormatActive( format ) {
+ return this.state.formats[ format ] && this.state.formats[ format ].isActive;


You can use lodash.get to simplify this code:

return get( this.state, [ 'formats', format, 'isActive' ], false );

I think this was copy paste from the original code at the time.

Another reason, to split this file into two chunks :)

gziolo · 2018-08-20T12:41:05Z

packages/editor/src/components/rich-text/format-toolbar/index.native.js

+ // const { linkValue, settingsVisible, opensInNewWindow } = this.state;
+ // const isAddingLink = formats.link && formats.link.isAdding;
+
+ const toolbarControls = FORMATTING_CONTROLS.concat( customControls )


It looks like all the difference between native and web in this file boils down to the render implementation. Can we extract another component instead and get rid of all this code duplication which quickly will become hard to maintain?

There is some import that are commented out as you can see on the top of the file. Should we go ahead and and create empty native version of those?

I would extract render method to its own component first and then see if there is anything else to override in the original file. My assumption is that all overrides happen inside render.

gziolo · 2018-08-20T12:42:47Z

packages/components/src/toolbar/index.js

 key={ [ indexOfSet, indexOfControl ].join() }
+ keyProp={ [ indexOfSet, indexOfControl ].join() }


keyProp is not needed in here, see my other comment.

gziolo · 2018-08-20T12:43:36Z

packages/components/src/toolbar/index.js

@@ -54,11 +56,12 @@ function Toolbar( { controls = [], children, className } ) {
 }

 return (
- <div className={ classnames( 'components-toolbar', className ) }>
+ <ToolbarContainer className={ classnames( 'components-toolbar', className ) }>


Can we convert div into View abstract component instead of reimplementing every div as a separate component?

👋 @gziolo, I deliberately avoided subplanting div with View because it would move the whole problem of mirroring a particular div's behavior to SCSS, and I don't think we should do that yet. SCSS support is still highly experimental in the RN app. So instead, I opted for implementing super specific components that are well defined in behavior, will be easy to reason about and will help us move along.

I actually expect patterns to start emerging the more divs we implement like that, and we'll start consolidating them into more general implementations, or better, indeed move them to general View+SCSS.

Can you create a follow up issue and leave inline comments so we make sure we clean it up when those patterns are established?

I'm AFK without a laptop for a couple of days (😱). I can do the inline comments in a few days unless someone else does it in the meantime. Feel free to do it @SergioEstevao or @gziolo?

A follow-up issue is fine, too. We can handle it, no worries @hypest.

Looping back, here's the ticket: #9284

gziolo · 2018-08-21T09:08:51Z

packages/keycodes/src/index.js

- *
- * @return {boolean} True if MacOS; false otherwise.
- */
-export function isMacOS( _window = window ) {


It looks like this method targets desktop Apple devices:
https://stackoverflow.com/questions/19877924/what-is-the-list-of-possible-values-for-navigator-platform-as-of-today

But window will not exist on a RN context correct?

Yes, you should omit it in RN override. I think it's done this way to be able to mock it in unit tests.

gziolo · 2018-08-22T07:04:24Z

I have just merged #9192 which should simplify this PR. There was related discussion if the mobile part is concerned at all with the link editing logic at the moment. We would like to move it to the LinkContainer component.

gziolo · 2018-08-22T11:46:21Z

packages/components/src/dashicon/index.js

@@ -9,6 +9,8 @@ OR if you're looking to change now SVGs get output, you'll need to edit strings
 * WordPress dependencies
 */
 import { Component } from '@wordpress/element';
+import SVG from './svg';


SVG and Path are internal dependencies.

gziolo · 2018-08-22T11:48:45Z

packages/components/src/tooltip/index.native.js

@@ -0,0 +1,4 @@
+import React from 'react';


We shouldn't be referencing React directly. We should rather import Children from @wordpress/element.

gziolo · 2018-08-22T11:50:41Z

packages/components/src/tooltip/index.native.js

@@ -0,0 +1,4 @@
+import React from 'react';
+
+// for the native mobile, just shortcircuit the Tooltip to return its child


This comment should follow convention and start with a capital letter and end with ..

gziolo · 2018-08-22T11:56:42Z

packages/editor/src/components/rich-text/format-toolbar/toolbar-container.native.js

+
+const ToolbarContainer = ( props ) => (
+ <View
+ style={ {flex: 1} }


Nit: there should be spaces inside inner curly braces

gziolo · 2018-08-22T11:57:08Z

packages/editor/src/components/rich-text/format-toolbar/toolbar-container.native.js

@@ -0,0 +1,10 @@
+import { View } from 'react-native'


Comment is missing:

/** * External dependencies */

gziolo · 2018-08-22T12:08:45Z

packages/editor/src/components/rich-text/format-toolbar/link-container.native.js

@@ -0,0 +1 @@
+export const LinkContainer = () => {}


default keyword is missing in the export statement

gziolo · 2018-08-22T12:09:10Z

packages/components/src/dashicon/svg.native.js

@@ -0,0 +1,7 @@
+import Svg from 'react-native-svg';


Comment is missing:

/** * External dependencies */

gziolo · 2018-08-22T12:09:14Z

packages/components/src/dashicon/path.native.js

@@ -0,0 +1,3 @@
+import { Path } from 'react-native-svg';


Comment is missing:

/** * External dependencies */

gziolo · 2018-08-22T12:09:37Z

packages/components/src/dashicon/index.js

@@ -9,6 +9,8 @@ OR if you're looking to change now SVGs get output, you'll need to edit strings
 * WordPress dependencies
 */
 import { Component } from '@wordpress/element';
+import SVG from './svg';


Both SVG and Path are internal dependencies

gziolo · 2018-08-22T12:10:48Z

packages/components/src/button/index.native.js

+ );
+}
+
+export default forwardRef( Button );


forwardRef won't work this way, besides it is not used inside the component. I would simply remove it from native implementation.

gziolo · 2018-08-22T12:12:13Z

packages/components/src/toolbar/index.js

- <div
- key={ [ indexOfSet, indexOfControl ].join() }
+ <ToolbarButtonContainer
+ key={ [ indexOfSet, indexOfControl ].join() } 


Nit: empty lines at the end of the line should be removed.

gziolo · 2018-08-22T14:23:22Z

packages/components/src/toolbar/toolbar-button-container.native.js

+
+export default ( props ) => (
+ <View
+ style={ props.className }


Will it work on mobile? It's a string but in other places I see objects.

…l keyboard

gziolo · 2018-08-22T14:50:09Z

package-lock.json

@@ -2652,6 +2653,13 @@
 "lodash": "^4.17.10"
 }
 },
+ "@wordpress/token-list": {


I'm not quite sure why package-json.lock got updated here.

I know what happened, @wordpress/token-list wasn't included in the package.json and it caused package-lock.json to be regenerated on 2nd npm install call...

/cc @aduth

gziolo

There are some minor questions to be answered or confirmed before merging but otherwise it looks good. Great work on this one. It was quite complex refactoring 👍

Can you retest with the Gutenberg mobile to ensure I didn't break anything with my recent commits?

…s to the lock file

…berg into rnmobile/toolbar

SergioEstevao added 4 commits August 14, 2018 18:46

Activate toolbar for RN.

911bbdc

Move toolbar to the top.

2390577

Implement RN icon buttons using SVG.

a497714

Add extra properties for toolbar selection.

f295934

SergioEstevao requested review from mtias, gziolo and hypest August 15, 2018 10:56

Update url package entry point for RN.

1b40eec

hypest added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Aug 16, 2018

hypest mentioned this pull request Aug 17, 2018

Leaner code for the native mobile Toolbar #9064

Merged

4 tasks

Leaner code for the native mobile Toolbar (#9064)

fefcb3b

* Dedicated components for SVG, Path * Fix lint errors * Dedicated native components for button, icon-button, tooltip * Dedicated native mobile comps for Toolbar containers * Small code styling fix * Fix double "key" typo in prop name

gziolo requested review from youknowriad, aduth and a team August 17, 2018 10:11

gziolo added the Framework Issues related to broader framework topics, especially as it relates to javascript label Aug 17, 2018

hypest added 2 commits August 17, 2018 13:47

Name the web side componets for better React debugging

45f2f85

Update the tests to cater for SVG, Path

470c7e6

Merge branch 'master' into rnmobile/toolbar

f9964b3

hypest mentioned this pull request Aug 17, 2018

Feature/toolbar wordpress-mobile/gutenberg-mobile#117

Merged

SergioEstevao mentioned this pull request Aug 20, 2018

Add support for react native WordPress/dashicons#315

Merged

gziolo requested changes Aug 20, 2018

View reviewed changes

SergioEstevao added 3 commits August 20, 2018 22:37

Deconstruct arguments in single line.

1d8c5d8

Remove comment.

c606b2e

Remove unnecessary use of keyProp

113df07

gziolo reviewed Aug 21, 2018

View reviewed changes

Fix use of platform for iOS detection.

1a919db

gziolo mentioned this pull request Aug 21, 2018

Editor: Extract LinkContainer from FormatToolbar #9192

Merged

4 tasks

Merge branch 'master' into rnmobile/toolbar

30cb9cc

SergioEstevao and others added 3 commits August 22, 2018 09:24

Merge branch 'master' into rnmobile/toolbar

b8123ec

Update toolbar RN code to share more code with Web.

c497a56

Update index.native.js

75b636d

gziolo reviewed Aug 22, 2018

View reviewed changes

SergioEstevao and others added 4 commits August 22, 2018 13:46

Address review comments.

b4eab29

Address eslint errors.

140e4fc

Remove extra white space.

587e576

Components: Add new primitives to work cross-platform

c89b1ab

gziolo reviewed Aug 22, 2018

View reviewed changes

gziolo added 2 commits August 22, 2018 16:45

Keycodes: Include support for both iPad and iPhone when using externa…

f8ed99d

…l keyboard

Components: Use explicit name for Tooltip component in RN

e603be2

gziolo reviewed Aug 22, 2018

View reviewed changes

gziolo approved these changes Aug 22, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into rnmobile/toolbar

8e72898

gziolo added this to the 3.7 milestone Aug 22, 2018

gziolo and others added 7 commits August 22, 2018 17:02

Add token-list as a dependency to package.json to avoid random update…

5f7f8ee

…s to the lock file

Return null to avoid failure on render.

faf1e0b

Merge branch 'rnmobile/toolbar' of https://github.com/WordPress/guten…

1f1b343

…berg into rnmobile/toolbar

Fix warning on SVG

8a55551

Fix potential error on style.

bb13473

Fix lint errors.

d3a8a45

Fix lint error.

b223fec

mzorz mentioned this pull request Aug 22, 2018

Feature/inserter modal wordpress-mobile/gutenberg-mobile#124

Merged

SergioEstevao merged commit 55cac4e into master Aug 22, 2018

SergioEstevao deleted the rnmobile/toolbar branch August 22, 2018 20:03

hypest mentioned this pull request Aug 23, 2018

Generalize the native mobile ports of DIVs #9284

Closed

		@@ -0,0 +1,4 @@
		// Components
		// eslint-disable-next-line camelcase

		key={ [ indexOfSet, indexOfControl ].join() }
		keyProp={ [ indexOfSet, indexOfControl ].join() }

		@@ -0,0 +1,4 @@
		import React from 'react';

		// for the native mobile, just shortcircuit the Tooltip to return its child

Implement toolbar on React Native #9012

Implement toolbar on React Native #9012

Conversation

SergioEstevao commented Aug 15, 2018

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

hypest commented Aug 17, 2018 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

hypest commented Aug 17, 2018 •

edited

Loading

gziolo Aug 21, 2018 •

edited

Loading

gziolo Aug 22, 2018 •

edited

Loading

gziolo Aug 22, 2018 •

edited

Loading