Skip to content
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

RichText: Use a simplified format for rich text values #7476

Closed
wants to merge 2 commits into from

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jun 22, 2018

This PR updates the RichText "element" format to use a simplified version of a React element instead of the whole object. This has the advantages:

  • Well defined format { type, props: { ...props, children }
  • The Children prop is consistent: always an array
  • This is a serializable format: It can be used as a comment attribute, it can also be used in templates from PHP.

Testing instructions

  • Check that writing in text blocks (heading, paragraph, list, tables...) still works as expected
  • Save and reload these blocks
  • Check some transforms.

closes #4489

related #771

@youknowriad youknowriad added the [Feature] Block API API that allows to express the block paradigm. label Jun 22, 2018
@youknowriad youknowriad self-assigned this Jun 22, 2018
@youknowriad youknowriad requested review from mcsf, aduth and ellatrix June 22, 2018 13:03
@aduth
Copy link
Member

aduth commented Jun 22, 2018

Few thoughts at a glance:

  • What's the return type of the node source? This simple element? Why do we have two names for the thing?
  • What guarantees do we have for compatibility between the simple element and @wordpress/element's ability to serialize it?
    • Speaking to: It's a bit odd that this is a "block" concept. Am I right in saying nothing that strictly forbids us to think of this as the canonical value shape of wp.element.createElement other than specific React internals making this prohibitively difficult?

@youknowriad
Copy link
Contributor Author

Speaking to: It's a bit odd that this is a "block" concept. Am I right in saying nothing that strictly forbids us to think of this as the canonical value shape of wp.element.createElement other than specific React internals making this prohibitively difficult?

That's true, what should we do in that case? Just move it to wp.element.createSimpleElement for now?

@ellatrix
Copy link
Member

I wouldn't quite say that this closes #771? It does help to get rid of staring the React element objects.

Why { type, props: { ...props, children } versus { type, props, ...children }?

@youknowriad
Copy link
Contributor Author

Why { type, props: { ...props, children } versus { type, props, ...children }?

To match React's shape essentially. Not certain I understand what's the content of children in your example though? (or maybe you meant to drop the ...)

This doesn't solve the "selection" state of RichText but solves only the value state from #771

@ellatrix
Copy link
Member

No the ... is supposed to be there, it would be an array. I'm merely wondering why that shape, that's all. The other shape might be a bit closer to JSX style.

@aduth
Copy link
Member

aduth commented Jun 26, 2018

There's an incidental performance benefit to this change, where currently we call updateContent in RichText many cases where content has not in-fact changed, merely because the assigned key is different. You can see this by pressing backspace from an empty new paragraph into a previous paragraph.

image

Having fewer properties is also more efficient a comparison.

@youknowriad
Copy link
Contributor Author

So what's remaining here? this maybe #7476 (comment)

@youknowriad youknowriad force-pushed the update/simplify-richtext-format branch from 90bfbd5 to 1634a05 Compare June 27, 2018 12:27
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { every, keys, isEqual, isFunction, isString } from 'lodash';
import { every, keys, isEqual, isFunction, isString, omit } from 'lodash';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable.

},
" text...",
{
"type": "br"
"type": "br",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny formatting? Running npm run fixtures:regenerates introduces a number of changes locally.

@@ -45,6 +46,24 @@ import serialize from './serialize';
*/
export { createElement };

/**
* Creates a simplified element representation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a simplified element, and why would I need one?

This is my main concern with this change, mostly that we have this separate concept of an element which is very incidental to a specific need for blocks, though could be a reasonable replacement for element altogether. So I don't think it's strictly unsuitable for @wordpress/element, though its existence parallel to the "full" element is unfortunate. The previous implementation where it existed in blocks was more accurate to the fact that it's an implementation detail therein, but then is complicated by the serializer's awareness of this shape.

I don't really have an answer here, just expressing thoughts in my head in response to your desire to move forward ( #7476 (comment) ).

Couple options to consider:

  • createElement should return the simplified form, and we somehow teach React to deal with it in rendering.
  • We move this back to blocks, but with some adapter which prepares an element for serialization, even if right now that just means returning the element verbatim.
    • Speaks also to the fact that we're misaligned between what React's renderer expects and our custom serializer can handle

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just consider this as a separate thing? It's just the HTML format of Gutenberg. Doesn't need to be coupled with element at all. Though I don't know what name to give it.

@aduth
Copy link
Member

aduth commented Jun 27, 2018

Another issue we'll need to address: Merging two paragraphs uses concatChildren, which itself uses React.Children.forEach, which validates the element shape and complains about our simpler object format.

@youknowriad
Copy link
Contributor Author

Superseded by #7934

@youknowriad youknowriad deleted the update/simplify-richtext-format branch July 20, 2018 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using Editable component when rendering serverside
3 participants