Introduces RichText component for mobile and ports Para block for mobile #8231

daniloercoli · 2018-07-26T14:30:01Z

Description

This PR introduces the RichText component for mobile, and implements the Paragraph Block by using it.

How has this been tested?

No tests implemented.

Types of changes

This PR adds a native version of the Paragraph Block, and a native version of the RichText component.

This new mobile RichText component doesn't use TinyMCE, but it's built over our React Native wrapper of Aztec editor(s). Being developed here https://github.com/wordpress-mobile/react-native-aztec, that's our port to the React world of our native editors, and referenced in this PR.

Checklist:

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

cc @SergioEstevao

… by using the newly introduced RichText component.

mzorz · 2018-07-26T16:26:10Z

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

+export default class RichText extends Component {
+	constructor() {
+		super( ...arguments );
+		//console.log('RichText');


We'd remove the commented lines

…b code.

…tive.js and relevant import statement.

…js`) in raw-handling

daniloercoli · 2018-07-27T13:37:54Z

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

+
+		this.currentTimer = setTimeout( function() {
+			// Create a React Tree from the new HTML
+			const newParaBlock = parse( '<!-- wp:paragraph --><p>' + this.lastContent + '</p><!-- /wp:paragraph -->' )[ 0 ];


We should change this asap, since will only work when RichText is used in a para block.

…ter-july # Conflicts: # core-blocks/paragraph/index.js

…rmation from HTML to React is made in the mobile paragraph block.

…block-use-latest-gb-master-july

hypest · 2018-08-01T10:42:36Z

👋 @SergioEstevao , can you add some documentation on the <p> replacement with <br>? I think a in-code comment will work well and I would suggest updating this PR's description to include it as a highlight. 🙇‍♂️

gziolo · 2018-08-01T10:42:01Z

core-blocks/paragraph/edit.native.js

+	}
+}
+
+const ParagraphEdit = function ParagraphEdit( { attributes, setAttributes } ) {


We don't need this one, you can export ParagraphBlock directly.

Aside: const ParagraphEdit = is obsolete in such code, it assigns the name to the function, but the function has already name specified.

gziolo · 2018-08-01T10:42:14Z

core-blocks/paragraph/edit.native.js

+					}
+					}
+					placeholder={ placeholder || __( 'Add text or type / to add content' ) }
+					aria-label={ __( 'test' ) }


Do we need this test to have included?

No, it's not needed at the moment.

gziolo · 2018-08-01T10:43:20Z

core-blocks/paragraph/edit.native.js

+			<View>
+				<RichText
+					content={ { contentTree: attributes.content, eventCount: attributes.eventCount } }
+					style={ style, [


This style definition is broken. Prop can take only one value so it probably doesn't take minHeight into account.

gziolo · 2018-08-01T10:44:13Z

core-blocks/paragraph/edit.native.js

+
+import { RichText } from '@wordpress/editor';
+
+const _minHeight = 50;


We don't use underscores to prepend private variables.

gziolo · 2018-08-01T10:44:50Z

core-blocks/paragraph/edit.js

+	}
+}
+
+const ParagraphEdit = compose( [


I noticed some differences withh the master branch. There were some changes applied to this code. I will fix them myself.

gziolo · 2018-08-01T10:48:03Z

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

+			content = <RawHTML>{ value }</RawHTML>;
+			break;
+
+		case 'element':


You don't need to port this deprecated code. It is going to be removed in the next version.

…obile paragraph block.

…React Native

…github.com:WordPress/gutenberg into rnmobile/port-para-block-use-latest-gb-master-july

gziolo · 2018-08-01T11:33:52Z

packages/blocks/src/api/raw-handling/phrasing-content.js

@@ -8,7 +8,7 @@ const phrasingContentSchema = {
 	em: {},
 	del: {},
 	ins: {},
-	a: { attributes: [ 'href' ] },
+	a: { attributes: [ 'href', 'target', 'rel' ] },


I noticed that this already get out of sync so I moved it to its own file.

gziolo

I added a few commits so I would appreciate another round of testing. Speaking myself this code looks good to merge on the web side.

gziolo · 2018-08-01T11:49:18Z

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

+ * External dependencies
+ */
+import RCTAztecView from 'react-native-aztec';
+import { children } from '@wordpress/blocks';


Nit: this import should be put under WordPress dependencies section.

gziolo · 2018-08-01T11:53:21Z

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

+		this.onChange = this.onChange.bind( this );
+		this.onContentSizeChange = this.onContentSizeChange.bind( this );
+
+		this.lastContentSizeHeight = -1;


For the next PR. I don't think it is reliable to use private variables inside component to keep state. You should be using this.setState method instead which React provides by default: https://reactjs.org/docs/faq-state.html.

Yeah, I used those private variables, instead keeping them in state, since even if the component gets recreated, and those variables reset to their initial value, the code works ok anyway. Those variables control if render should be executed or not.

gziolo · 2018-08-01T11:55:00Z

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

+		return true;
+	}
+
+	setForceUpdate( flag ) {


This is never used in this code, can you explain why you need it?

gziolo · 2018-08-01T11:56:07Z

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

+
+	onContentSizeChange( event ) {
+		this.lastContentSizeHeight = event.nativeEvent.contentSize.height;
+		this.forceUpdate = true; // Set this to true and check it in shouldComponentUpdate to force re-render the component


React provides built-in method which does the same forceUpdate: https://reactjs.org/docs/react-component.html#forceupdate.

gziolo · 2018-08-01T11:57:54Z

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

+	}
+
+	shouldComponentUpdate( nextProps ) {
+		if ( !! this.forceUpdate ) {


There is no need to convert values to boolean in JS. this.forceUpdate is all you need here and also it works the same in other cases below. JavaScript engine does type coercion itself.

…ersion of value to boolean in if. JS does it itself.

…ethod variable.

…olean variable used to skip the rendering.

daniloercoli · 2018-08-01T14:41:17Z

Thanks @gziolo for the detailed review, and suggestions! 🙇

daniloercoli added 2 commits July 26, 2018 15:36

Introduce RichText component for mobile, and port the Paragraph block…

3318475

… by using the newly introduced RichText component.

Fix lint

1ffdf7c

daniloercoli added Mobile Web Viewport sizes for mobile and tablet devices Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Jul 26, 2018

daniloercoli added this to the 3.4 milestone Jul 26, 2018

daniloercoli requested review from gziolo and hypest July 26, 2018 14:30

daniloercoli mentioned this pull request Jul 26, 2018

Introduce paragraph block and RichText component for mobile wordpress-mobile/gutenberg-mobile#81

Merged

daniloercoli removed the Mobile Web Viewport sizes for mobile and tablet devices label Jul 26, 2018

mzorz reviewed Jul 26, 2018

View reviewed changes

SergioEstevao and others added 3 commits July 26, 2018 18:04

Use an edit file to share more common code with the main Paragraph we…

73d1a38

…b code.

Fix lint

1669996

Remove log calls

ec0f897

SergioEstevao mentioned this pull request Jul 27, 2018

Ports the Paragraph Block to React Native #8149

Closed

4 tasks

daniloercoli added 2 commits July 27, 2018 11:43

Revert renaming of custom-class-name.native.js → custom-class-name-na…

20d7082

…tive.js and relevant import statement.

Cleaning the code / Refactoring by "nativizing" utils.js (& `index.…

b918423

…js`) in raw-handling

daniloercoli commented Jul 27, 2018

View reviewed changes

SergioEstevao added 2 commits July 27, 2018 14:51

Remove any extra P.

3348926

Replace <p> by <br>

804b52a

pento modified the milestones: 3.4, 3.5 Jul 30, 2018

SergioEstevao and others added 3 commits July 30, 2018 14:13

Merge branch 'master' into rnmobile/port-para-block-use-latest-gb-mas…

8e75df3

…ter-july # Conflicts: # core-blocks/paragraph/index.js

The RichText mobile component now returns plain HTML, and the transfo…

bed493b

…rmation from HTML to React is made in the mobile paragraph block.

Merge remote-tracking branch 'origin/master' into rnmobile/port-para-…

c9373d2

…block-use-latest-gb-master-july

gziolo reviewed Aug 1, 2018

View reviewed changes

gziolo and others added 4 commits August 1, 2018 12:52

Update code to work with the lastest master changes

f0e83ec

Correctly check if minHeight is undefined when setting the style of m…

b275731

…obile paragraph block.

Blocks: Extract phrasing content to its own file to allow reuse with …

f214c76

…React Native

Merge branch 'rnmobile/port-para-block-use-latest-gb-master-july' of …

762b582

…github.com:WordPress/gutenberg into rnmobile/port-para-block-use-latest-gb-master-july

gziolo reviewed Aug 1, 2018

View reviewed changes

gziolo approved these changes Aug 1, 2018

View reviewed changes

gziolo reviewed Aug 1, 2018

View reviewed changes

gziolo assigned daniloercoli and SergioEstevao Aug 1, 2018

daniloercoli and others added 5 commits August 1, 2018 14:54

Remove method setForceUpdate that is not used. Also remove the conv…

9651c70

…ersion of value to boolean in if. JS does it itself.

Remove aria-label that is not used in RichText mobile

248a2c8

Convert private instance variable lastContentSizeHeight, to local m…

b34049a

…ethod variable.

Use built-in forceUpdate method call, instead of using a private bo…

131e3e7

…olean variable used to skip the rendering.

Add comment for P replacement for BR

3351260

daniloercoli merged commit 1c966a9 into master Aug 1, 2018

daniloercoli deleted the rnmobile/port-para-block-use-latest-gb-master-july branch August 1, 2018 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces RichText component for mobile and ports Para block for mobile #8231

Introduces RichText component for mobile and ports Para block for mobile #8231

daniloercoli commented Jul 26, 2018 •

edited

Loading

mzorz Jul 26, 2018

daniloercoli Jul 27, 2018

hypest commented Aug 1, 2018

gziolo Aug 1, 2018

gziolo Aug 1, 2018

daniloercoli Aug 1, 2018

gziolo Aug 1, 2018

gziolo Aug 1, 2018

gziolo Aug 1, 2018

gziolo Aug 1, 2018

gziolo Aug 1, 2018

gziolo left a comment

gziolo Aug 1, 2018

gziolo Aug 1, 2018

daniloercoli Aug 1, 2018

gziolo Aug 1, 2018

gziolo Aug 1, 2018

gziolo Aug 1, 2018

daniloercoli commented Aug 1, 2018


		import { RichText } from '@wordpress/editor';

		const _minHeight = 50;

Introduces RichText component for mobile and ports Para block for mobile #8231

Introduces RichText component for mobile and ports Para block for mobile #8231

Conversation

daniloercoli commented Jul 26, 2018 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest commented Aug 1, 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

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

daniloercoli commented Aug 1, 2018

daniloercoli commented Jul 26, 2018 •

edited

Loading