Editable: state as a cleaned up tree #3048

ellatrix · 2017-10-18T09:31:58Z

Description

Tries to address #2750.

This branch was created as a proof of concept and alternative to #2786. Logging it as a PR, but it's still a work-in-progress.

The goal is to have the editable state as a true (simple arrays), a cleaned up version of the current state as a React tree.

ellatrix · 2017-10-19T13:50:06Z

While quickly putting this alternative together, I used dom-react to loop through the nodes. I wonder if we want to keep this though. The most important thing it does is mapping attributes to React canonical ones and parse the style attribute to JSON. If we don't mind attributes with dashes in the state... we could loop ourselves and drop this. With React 16, it seems still recommended to use the canonical attributes, so then we would need to map from state to React, instead of dom to React.

ellatrix · 2017-10-19T13:52:35Z

blocks/editable/test/index.js

+		} );
+
+		it( 'should render null when false passed as value', () => {
+			const wrapper = shallow( <Editable.Value value={ [ false ] } /> );


Is this needed? Not sure how that could happen.

(Same for the above.)

We can drop it, I just wanted to track down why this fails in test env.

gziolo · 2017-10-19T14:06:08Z

It turns out we are using React 15 and this break most of the code used. I will look into an upgrade to React 16 tomorrow to get rid of <span /> workaround.

aduth · 2017-10-19T20:06:40Z

blocks/editable/index.js

+		return createElement( type, { ...props, key: i }, valueToReact( children ) );
+	} );
+
+	// We are still using React 15, so can't return array or string directly...


Only in test environment are we using React 15.

Hopefully not that long: #3079 :)

gziolo · 2017-10-20T10:17:36Z

As soon as we land React upgrade for tests (#3079) I will update PR to stop using hacky <span /> wrapper.

gziolo · 2017-10-20T17:35:14Z

I rebased with master branch which now has React 16 and Enzyme 3.

We have still 14 failing tests, but 12 of them are directly related to the different JSON representation of components. There are 2 tests failing that we still need to investigate:

FAIL blocks/api/test/source.js
sources
✓ should generate sources which apply internal flag (7ms)
children()
✓ should return a source function (1ms)
✕ should return HTML equivalent WPElement of matched element (10ms)
node()
✓ should return a source function (1ms)
✕ should return HTML equivalent WPElement of matched element (2ms)

● sources › children() › should return HTML equivalent WPElement of matched element

Invariant Violation: Objects are not valid as a React child (found: object with keys {}). If you meant to render a collection of children, use an array instead.null
  
  at invariant (node_modules/react/node_modules/fbjs/lib/invariant.js:42:15)
  at traverseAllChildrenImpl (node_modules/react/cjs/react.development.js:830:7)
  at traverseAllChildrenImpl (node_modules/react/cjs/react.development.js:803:23)
  at traverseAllChildrenImpl (node_modules/react/cjs/react.development.js:803:23)
  at traverseAllChildren (node_modules/react/cjs/react.development.js:858:10)
  at mapIntoWithKeyPrefixInternal (node_modules/react/cjs/react.development.js:934:3)
  at toArray (node_modules/react/cjs/react.development.js:981:3)
  at new ReactDOMServerRenderer (node_modules/react-dom/cjs/react-dom-server.node.development.js:2677:64)
  at renderToStaticMarkup (node_modules/react-dom/cjs/react-dom-server.node.development.js:2990:18)
  at Object.<anonymous> (blocks/api/test/source.js:36:39)
  at process._tickCallback (internal/process/next_tick.js:109:7)

● sources › node() › should return HTML equivalent WPElement of matched element

Invariant Violation: Objects are not valid as a React child (found: object with keys {}). If you meant to render a collection of children, use an array instead.null
  
  at invariant (node_modules/react/node_modules/fbjs/lib/invariant.js:42:15)
  at traverseAllChildrenImpl (node_modules/react/cjs/react.development.js:830:7)
  at traverseAllChildrenImpl (node_modules/react/cjs/react.development.js:803:23)
  at traverseAllChildren (node_modules/react/cjs/react.development.js:858:10)
  at mapIntoWithKeyPrefixInternal (node_modules/react/cjs/react.development.js:934:3)
  at toArray (node_modules/react/cjs/react.development.js:981:3)
  at new ReactDOMServerRenderer (node_modules/react-dom/cjs/react-dom-server.node.development.js:2677:64)
  at renderToStaticMarkup (node_modules/react-dom/cjs/react-dom-server.node.development.js:2990:18)
  at Object.<anonymous> (blocks/api/test/source.js:53:39)
  at process._tickCallback (internal/process/next_tick.js:109:7)

gziolo · 2017-10-23T14:40:59Z

blocks/api/test/source.js

 			const match = parse( html, sources.node() );

-			expect( renderToString( match ) ).toBe( `<body>${ html }</body>` );
+			expect( renderToString( valueToElement( [ match ] ) ) ).toBe( `<body>${ html }</body>` );


@iseulde this test is failing. I didn't find a way to fix it. There is some issue with the sources.node parser. It inlines string nodes and nodes represented as an array. Once this is sorted out we should be good to do.

It also fails for this HTML fixture.

<blockquote class="wp-block-pullquote alignnone"> <p>Paragraph <strong>one</strong></p> <p>Paragraph two</p> <footer>by whomever</footer> </blockquote>

I think I fixed it :)

gziolo · 2017-10-24T10:19:46Z

blocks/library/table/index.js

@@ -27,10 +28,29 @@ registerBlockType( 'core/table', {
 			type: 'array',
 			source: children( 'table' ),
 			default: [


We might want to keep what we had before and provide transformation layer instead. I wanted to make it work.

gziolo · 2017-10-24T10:28:18Z

@iseulde @aduth @youknowriad - I manage to make it work with both the existing unit tests and the browser. There ~~are two~~ is one known issue:

Transforming from the quote or pullquote to anything else breaks the block.
~~It's not possible to use toggle on anchor until it's saved.~~ (I can see it also in the master branch)

youknowriad · 2017-10-24T10:14:41Z

blocks/api/source.js

@@ -44,7 +43,7 @@ export const children = withKnownSourceFlag( ( selector ) => {
 		}

 		if ( match ) {
-			return nodeListToReact( match.childNodes || [], createElement );
+			return nodeListToReact( match.childNodes || [], toArray );


I guess dom-react should be something like dom-traverse?

Should I read it as we should introduce a facade for dom-react? I think it would make a lot of sense.

I guess dom-react should be something like dom-traverse?

Yeah, this confused me as well, and I thought we were creating instances of React elements and then converting those into arrays. Understand better now, but the naming is a bit misleading. I also wonder how much of the React-isms we really need to be accounting for (e.g. key) or if it's enough to pick attributes from node.attributes (see 6521f06).

Honestly I just did this to spin up a fast POC. Would prefer if we get rid of this too.

youknowriad · 2017-10-24T10:17:46Z

blocks/editable/index.js

@@ -36,7 +36,7 @@ import { EVENTS } from './constants';

 const { BACKSPACE, DELETE, ENTER } = keycodes;

-function createTinyMCEElement( type, props, ...children ) {
+function toArray( type, props, ...children ) {


Should we name it valueFromElement (to be consistent with valueToElement)?

Good idea, I like your proposal :)

ellatrix · 2017-10-24T11:53:48Z

blocks/api/source.js

@@ -58,6 +57,6 @@ export const node = withKnownSourceFlag( ( selector ) => {
 			match = domNode.querySelector( selector );
 		}

-		return nodeToReact( match, createElement );
+		return flatten( nodeToReact( match, toArray ) );


Why does this need to flatten?

Yes, this one is tricky. I had to flatten it to mirror what is happening in the editor. Somehow it wraps with one additional array when you compare with what is passed to <Editable /> or <Editable.value />. It was quite hard to find a setup where all moving parts are satisfied: <Editable />, <Editable.value />, pasting from other documents. It might be a better idea to wrap value with another array in Editable. I'm not sure what's better here.

gziolo · 2017-12-13T09:59:28Z

blocks/editable/index.js

+		const [ type, props, ...children ] = element;
+		const reactProps = Object.keys( props ).reduce( ( accumulator, key ) => {
+			const mapping = {
+				class: 'className',


It looks like adding those 2 mappings is enough to satisfy React.

Okey, style needs its own handling, because I see this: https://reactjs.org/docs/error-decoder.html?invariant=62&args[]=

All keys must go camel case, too.

gziolo · 2017-12-13T10:00:18Z

blocks/editable/index.js

+			};
+		}, { key: i } );
+
+		return createElement( type, { ...reactProps }, ...valueToElement( children ) );


It can be simply reactProps :)

gziolo · 2017-12-13T10:02:42Z

blocks/editable/index.js

@@ -850,7 +882,7 @@ export default class Editable extends Component {
 					getSettings={ this.getSettings }
 					onSetup={ this.onSetup }
 					style={ style }
-					defaultValue={ value }
+					defaultValue={ valueToElement( value ) }


Can we use applySimpleNodeList in here, too?

aduth · 2017-12-13T19:55:44Z

@gziolo @iseulde How reasonable do you think (in parallel to the work here) to implement our own save serialization and skip React's altogether? Maybe similar to the ideas in #2463 with interoperability renderers. That way, we might not need Editable.Value or otherwise coercing the value back to a React element, if we had built-in understanding of this tree shape.

I'm finding we're repeatedly bumping up against issues with React's serialization, most recently summarized at #3353 (comment).

ellatrix · 2017-12-13T22:03:38Z

@aduth But then we can't use React elements inside the save function? Or would it be mixed?

aduth · 2017-12-14T13:29:38Z

@aduth But then we can't use React elements inside the save function? Or would it be mixed?

Right, I would think still supporting React elements, but "turning off" some of the sanitization behaviors, and supporting other object shapes.

ellatrix · 2018-01-10T16:27:35Z

Should we consider this now as succeeded by #4049?

gziolo · 2018-01-10T17:02:50Z

Should we consider this now as succeeded by #4049?

Yes, probably it makes the most sense. This one was very helpful in the context of exploring what we really need. We should focus on one approach and finally get it merged :D

ellatrix · 2018-01-11T09:53:31Z

Closing for now in favour of #4049.

ellatrix added [Status] In Progress Tracking issues with work in progress [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Oct 18, 2017

gziolo assigned ellatrix and gziolo Oct 19, 2017

gziolo force-pushed the try/editable-state-tree branch 4 times, most recently from 6f18bbd to e6ffd52 Compare October 19, 2017 13:49

ellatrix commented Oct 19, 2017

View reviewed changes

gziolo force-pushed the try/editable-state-tree branch from e6ffd52 to 14315b3 Compare October 19, 2017 14:25

aduth reviewed Oct 19, 2017

View reviewed changes

gziolo force-pushed the try/editable-state-tree branch from 14315b3 to eebaeca Compare October 20, 2017 17:32

gziolo force-pushed the try/editable-state-tree branch from eebaeca to 0b5428f Compare October 23, 2017 11:31

gziolo reviewed Oct 23, 2017

View reviewed changes

gziolo force-pushed the try/editable-state-tree branch 3 times, most recently from 4cf568b to 03c9bc0 Compare October 24, 2017 10:11

WordPress deleted a comment from codecov bot Oct 24, 2017

gziolo requested a review from youknowriad October 24, 2017 10:13

gziolo added Framework Issues related to broader framework topics, especially as it relates to javascript and removed [Status] In Progress Tracking issues with work in progress labels Oct 24, 2017

gziolo reviewed Oct 24, 2017

View reviewed changes

gziolo requested a review from mtias October 24, 2017 11:16

youknowriad reviewed Oct 24, 2017

View reviewed changes

ellatrix commented Oct 24, 2017

View reviewed changes

aduth mentioned this pull request Dec 11, 2017

Blocks: Support block nesting (Add Columns block, InnerBlocks component) #3745

Merged

ellatrix and others added 10 commits December 13, 2017 09:50

WIP

2bfc296

Editable: Add missing Editable.Value wrappers to the existing blocks

7d243fa

Editable: Add unit tests to verify Editable.Value

0662a94

Testing: Regenerate test fixtures for full post content checks

2a0611e

Testing: Fix failing tests caused by the node matcher

2a1989c

Framework: Final touches to allow saving blocks state as a tree

54d7631

Fix eslint error

e50084a

Fix docs

7177a6d

Try without dom-react

9dab041

WIP

df23eea

gziolo force-pushed the try/editable-state-tree branch 3 times, most recently from 525b9c4 to 107a6d9 Compare December 13, 2017 09:53

Editable: Use valueToElement to render stored tree representation

e9b7111

gziolo force-pushed the try/editable-state-tree branch from 107a6d9 to e9b7111 Compare December 13, 2017 09:57

gziolo reviewed Dec 13, 2017

View reviewed changes

aduth mentioned this pull request Dec 13, 2017

Editor: straight quotes don't filter to curly quotes on publish as expected #3353

Closed

aduth mentioned this pull request Dec 15, 2017

Try: Custom save serializer, Editable as array tree #4049

Closed

gziolo removed their assignment Jan 10, 2018

ellatrix closed this Jan 11, 2018

ellatrix deleted the try/editable-state-tree branch January 11, 2018 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editable: state as a cleaned up tree #3048

Editable: state as a cleaned up tree #3048

ellatrix commented Oct 18, 2017 •

edited by gziolo

Loading

ellatrix commented Oct 19, 2017

ellatrix Oct 19, 2017 •

edited

Loading

ellatrix Oct 19, 2017

gziolo Oct 19, 2017

gziolo commented Oct 19, 2017

aduth Oct 19, 2017

gziolo Oct 20, 2017

gziolo commented Oct 20, 2017 •

edited

Loading

gziolo commented Oct 20, 2017

gziolo Oct 23, 2017

gziolo Oct 24, 2017

gziolo Oct 24, 2017

gziolo commented Oct 24, 2017 •

edited

Loading

youknowriad Oct 24, 2017

gziolo Oct 24, 2017

aduth Oct 26, 2017

ellatrix Oct 30, 2017

youknowriad Oct 24, 2017

gziolo Oct 24, 2017

ellatrix Oct 24, 2017

gziolo Oct 24, 2017

gziolo Dec 13, 2017

gziolo Dec 13, 2017

gziolo Dec 13, 2017

gziolo Dec 13, 2017

gziolo Dec 13, 2017

aduth commented Dec 13, 2017

ellatrix commented Dec 13, 2017

aduth commented Dec 14, 2017

ellatrix commented Jan 10, 2018

gziolo commented Jan 10, 2018

ellatrix commented Jan 11, 2018

Editable: state as a cleaned up tree #3048

Editable: state as a cleaned up tree #3048

Conversation

ellatrix commented Oct 18, 2017 • edited by gziolo Loading

Description

ellatrix commented Oct 19, 2017

ellatrix Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Oct 20, 2017 • edited Loading

gziolo commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Oct 24, 2017 • 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

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

aduth commented Dec 13, 2017

ellatrix commented Dec 13, 2017

aduth commented Dec 14, 2017

ellatrix commented Jan 10, 2018

gziolo commented Jan 10, 2018

ellatrix commented Jan 11, 2018

ellatrix commented Oct 18, 2017 •

edited by gziolo

Loading

ellatrix Oct 19, 2017 •

edited

Loading

gziolo commented Oct 20, 2017 •

edited

Loading

gziolo commented Oct 24, 2017 •

edited

Loading