Try avoiding parse to get content of Editable #523

ellatrix · 2017-04-27T12:21:21Z

This is just an experiment. Creating this branch to see what others think.

The current behaviour is DOM => HTML => DOM => VDOM (React). This change proposes DOM => VDOM (React) which would eliminate HTML => DOM parsing every time getContent is called.

WIP

aduth

I like the idea. 👍 Seems everywhere we're using html-to-react we have access to a DOM node anyways, so a nice optimization to skip the step of converting the string to a DOM.

aduth · 2017-04-27T13:33:55Z

blocks/components/editable/dom-to-react.js

@@ -0,0 +1,29 @@
+import { createElement } from 'react';


Since we'll need this in blocks/api/query.js as well, we'll probably want to place the file somewhere more generic. Kinda ties to #408, as it seems in addition to component components, we'll need common utilities as well.

That, or we could just commit to individually publishing npm modules for the utilities we create.

That, or we could just commit to individually publishing npm modules for the utilities we create.

If we don't depend on html-to-react anyway, I was thinking of just creating a separate package that has some options to handle nodes (like we need to ignore internal mce stuff).

ellatrix · 2017-04-27T13:42:39Z

If this works well, I'll go ahead and create a package for it.

youknowriad · 2017-04-27T13:48:59Z

blocks/components/editable/index.js

-				htmlToReactParser.parse( before ),
-				htmlToReactParser.parse( after )
+				map( before, nodeToReact ),
+				hasAfter ? map( childNodes.slice( splitIndex ), nodeToReact ) : ''


I think the fallback value shouldn't be a string here?

ellatrix · 2017-04-27T13:56:30Z

@aduth What do you have in mind for https://github.com/WordPress/gutenberg/blob/master/blocks/api/query.js?

aduth · 2017-04-27T14:05:04Z

It's not obvious because we use flow there, but the signature for the return value of a matcher is ( node: Element ): mixed, so we could change it to:

export function children( selector ) {
	return ( node ) => {
		let match = node;
		if ( selector ) {
			match = node.querySelector( selector );
		}

		return nodeToReact( match );
	};
}

ellatrix · 2017-04-27T14:39:01Z

For me this seems to be working well now.

aduth · 2017-04-27T18:37:03Z

blocks/components/editable/dom-to-react.js

+		return node.nodeValue;
+	}
+
+	if ( node.getAttribute( 'data-mce-bogus' ) === 'all' ) {


What's the plan for handling this and the below key.startsWith( 'data-mce-' ) when we make the utility more generic?

Yeah, that's the next thing :)

So right now you can pass the createElement callback, which means that you can filter stuff, there is no dependency on React, and you can use it with other React-like libraries.

aduth · 2017-04-27T18:39:02Z

blocks/components/editable/dom-to-react.js

+	const props = reduce( node.attributes, ( result, { name, value } ) => {
+		let key = camelCaseAttrMap[ name.replace( /[-:]/, '' ) ] || name;
+
+		if ( key.startsWith( 'data-mce-' ) ) {


We don't use babel-polyfill, so we don't have access to ES2015 base type prototype methods like Array#includes or String#startsWith. This will error in IE11. We may consider incorporating the polyfill. It's not small though. But this is a common enough problem that it's probably worth considering since it's easy for a developer to overlook. That or some custom lint behaviors.

Oh, didn't realise this.

aduth · 2017-04-27T18:40:15Z

blocks/api/query.js

+			match = node.querySelector( selector );
+		}
+
+		if ( match ) {


Why can't we just return nodeToReact( match );?

Don't we need the childNodes here?

Don't we need the childNodes here?

Yeah, you're right.

ellatrix · 2017-04-28T11:03:05Z

Just to clarify; I believe data-mce-bogus=all means that the whole element and its children should be ignored, data-mce-bogus means that the element itself should be ignored, but not its children, and any data-mce-* attributes should be ignored. Does this sound right @spocke?

aduth · 2017-04-28T17:10:51Z

Maybe an option could be created for nodeToReact to provide hooks for mapping nodes and attributes?

nodeListToReact( element, {
	mapNode: ( node ) => {
		switch ( node.getAttribute( 'data-mce-bogus' ) ) {
			case 'all': return null;
			case null: return node;
			default: return node.childNodes;
		}
	},
	mapAttribute: ( value, key ) => startsWith( 'data-mce-', key ) ? null : value
} );

ellatrix · 2017-04-28T17:32:11Z

You don't like the current solution? :)

aduth · 2017-04-28T17:39:39Z

Oh, I think I missed the transition to passing in createElement and thought you'd reimplemented a separate nodeListToReact in the Editable. At a glance, yeah, this seems like a nicer way to hook the element creation itself instead.

ellatrix · 2017-04-28T18:39:23Z

Yeah, it's also nice that it's very flexible, you can use it with other React-like libraries.

ellatrix · 2017-04-28T20:11:49Z

I added style support in the package, and this seems to be working well. Any concerns, thoughts? Mergeable?

aduth

Needs a rebase, but otherwise looks good 👍

youknowriad mentioned this pull request Apr 27, 2017

Text Block: Fix updating the content of an empty text block #512

Merged

aduth added the [Status] In Progress Tracking issues with work in progress label Apr 27, 2017

aduth reviewed Apr 27, 2017

View reviewed changes

youknowriad reviewed Apr 27, 2017

View reviewed changes

ellatrix mentioned this pull request Apr 27, 2017

Add placeholder functionality to Editable component #475

Closed

aduth reviewed Apr 27, 2017

View reviewed changes

ellatrix force-pushed the try/dom-to-react branch from 60c5c68 to c1e4aca Compare April 28, 2017 18:52

ellatrix removed the [Status] In Progress Tracking issues with work in progress label Apr 28, 2017

aduth approved these changes May 1, 2017

View reviewed changes

ellatrix added 7 commits May 1, 2017 20:08

Try avoiding parse to get content of Editable

e4f7c2f

Remove parsing on split

ebb70c8

Ignore internal attributes

7e07869

use dom-to-react for query.children

5ebdd32

Move dom-react to npm package

370d799

Ignore internal attributes

c168114

Fix linting errors

c56e6a0

ellatrix force-pushed the try/dom-to-react branch from c1e4aca to c56e6a0 Compare May 1, 2017 18:15

ellatrix merged commit 174fdf1 into master May 1, 2017

aduth mentioned this pull request May 1, 2017

When blocks are emptied in content, TinyMCE passes in an empty <br /> to the Redux State. #561

Closed

mtias deleted the try/dom-to-react branch May 1, 2017 18:45

aduth mentioned this pull request May 5, 2017

Use raw format when setting Editable content #667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try avoiding parse to get content of Editable #523

Try avoiding parse to get content of Editable #523

ellatrix commented Apr 27, 2017

aduth left a comment

aduth Apr 27, 2017

ellatrix Apr 27, 2017

ellatrix commented Apr 27, 2017

youknowriad Apr 27, 2017

ellatrix Apr 27, 2017

ellatrix commented Apr 27, 2017

aduth commented Apr 27, 2017

ellatrix commented Apr 27, 2017

aduth Apr 27, 2017

ellatrix Apr 27, 2017

ellatrix Apr 28, 2017

aduth Apr 27, 2017

ellatrix Apr 27, 2017

aduth Apr 27, 2017

ellatrix Apr 27, 2017

aduth Apr 28, 2017

ellatrix commented Apr 28, 2017

aduth commented Apr 28, 2017

ellatrix commented Apr 28, 2017 •

edited

Loading

aduth commented Apr 28, 2017

ellatrix commented Apr 28, 2017

ellatrix commented Apr 28, 2017

aduth left a comment

Try avoiding parse to get content of Editable #523

Try avoiding parse to get content of Editable #523

Conversation

ellatrix commented Apr 27, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Apr 27, 2017

aduth commented Apr 27, 2017

ellatrix commented Apr 27, 2017

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

ellatrix commented Apr 28, 2017

aduth commented Apr 28, 2017

ellatrix commented Apr 28, 2017 • edited Loading

aduth commented Apr 28, 2017

ellatrix commented Apr 28, 2017

ellatrix commented Apr 28, 2017

aduth left a comment

Choose a reason for hiding this comment

ellatrix commented Apr 28, 2017 •

edited

Loading