Skip to content

RichText: Add a format prop to allow HTML string values to be used in RichText components #6034

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.

Sign up for GitHub

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

Merged
youknowriad merged 8 commits into from
Apr 19, 2018
Merged

RichText: Add a format prop to allow HTML string values to be used in RichText components #6034

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cf196aa
RichText: Add a format prop to allow HTML string values to be used in…
youknowriad Apr 6, 2018
d85b718
Don't align params and returns docs
youknowriad Apr 11, 2018
a3f4726
Add domToFormat function to factorize ternaries
youknowriad Apr 11, 2018
0bec108
Remove useless stringToElement
youknowriad Apr 11, 2018
dd9e099
Add valueToString to avoid ternaries when converting to strings
youknowriad Apr 11, 2018
2963b65
Avoid double isEmpty check
youknowriad Apr 13, 2018
665c59e
Use valid HTML in unit tests
youknowriad Apr 13, 2018
db26521
Fix splitting
ellatrix Apr 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add domToFormat function to factorize ternaries
  • Loading branch information
@youknowriad @ellatrix
youknowriad authored and ellatrix committed Apr 16, 2018
commit a3f4726a3f45de4eaa78309a21bff3c623bcb238
17 changes: 17 additions & 0 deletions blocks/rich-text/format.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,20 @@ export function domToElement( value ) {
export function domToString( value ) {
return map( value, element => element.outerHTML ).join( '' );
Copy link
Member

@ellatrix ellatrix Apr 13, 2018

Choose a reason for hiding this comment

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

In domToElement, we're always stripping any TinyMCE internal element and attributes, but here we're not, even though there might be cases where we receive some in the case of splitting.

Copy link
Contributor Author

@youknowriad youknowriad Apr 13, 2018

Choose a reason for hiding this comment

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

Not sure how to address this in an optimal way. Don't we have tools in TinyMCE to do this for us instead of adding custom logic? Is it a requirement since we're not using this when getting the whole content of the editor but only when splitting?

Copy link
Member

@aduth aduth Apr 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

( new tinymce.dom.Serializer( {} ) ).serialize( el ) ? (Likely with some reuse of a serializer instance)

Copy link
Contributor Author

@youknowriad youknowriad Apr 13, 2018

Choose a reason for hiding this comment

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

Thanks Andrew but unfortunately it's not working in my testing, we more likely have to specify the same rules used by TinyMCE internally

Copy link
Member

@ellatrix ellatrix Apr 13, 2018

Choose a reason for hiding this comment

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

Is it a requirement since we're not using this when getting the whole content of the editor but only when splitting?

Well, the content will end up getting passed to the store, so I guess so. On consecutive edits we may pass getContent again, but this doesn't necessarily happen. Say you split a paragraph in two and the first part contains some elements with internal attributes. This will be passed on eventually to (block) setAttributes.

Not sure immediately how to fix either but I guess @aduth goes in the right direction. Maybe parse and serialise if you want to work on the string? I'll have a look if we can work on the fragment.

Copy link
Member

@ellatrix ellatrix Apr 13, 2018

Choose a reason for hiding this comment

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

Did a little test here: 39d9966

Splitting is currently entirely broken with the HTML format. The diff fixes it and should also get rid of internal stuff.

Copy link
Member

@ellatrix ellatrix Apr 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

After the splitting fix, but without serialising, you'd get this:

The goal of this new editor is to make adding rich content to WordPress simple and enjoyable. This whole post is composed of <em data-mce-selected="inline-boundary">pieces of</em>

when splitting the first paragraph of the demo content.

Copy link
Member

@ellatrix ellatrix Apr 13, 2018

Choose a reason for hiding this comment

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

(The splitting is broken because element.outerHTML doesn't work on text nodes that are passed.)

}

/**
* Transforms an array of DOM Elements to the given format.
*
* @param {Array} value DOM Elements.
* @param {string} format Output format (string or element)
*
* @return {*} Output.
*/
export function domToFormat( value, format ) {
switch ( format ) {
case 'string':
return domToString( value );
default:
return domToElement( value );
}
}
17 changes: 8 additions & 9 deletions blocks/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { pickAriaProps } from './aria';
import patterns from './patterns';
import { EVENTS } from './constants';
import { withBlockEditContext } from '../block-edit/context';
import { domToElement, domToString, elementToString } from './format';
import { domToString, elementToString } from './format';

const { BACKSPACE, DELETE, ENTER } = keycodes;

Expand Down Expand Up @@ -476,8 +476,8 @@ export class RichText extends Component {
const afterNodes = childNodes.slice( index + 1 );

const { format } = this.props;
const before = format === 'string' ? domToString( beforeNodes ) : domToElement( beforeNodes );
const after = format === 'string' ? domToString( afterNodes ) : domToElement( afterNodes );
const before = domToFormat( beforeNodes, format );
const after = domToFormat( afterNodes, format );

this.restoreContentAndSplit( before, after );
} else {
Expand Down Expand Up @@ -571,9 +571,8 @@ export class RichText extends Component {
const afterFragment = afterRange.extractContents();

const { format } = this.props;
const before = format === 'string' ? domToString( beforeFragment.childNodes ) : domToElement( beforeFragment.childNodes );
const filteredAfterFragment = filterEmptyNodes( afterFragment.childNodes );
const after = format === 'string' ? domToString( filteredAfterFragment ) : domToElement( filteredAfterFragment );
const before = domToFormat( beforeFragment.childNodes, format );
const after = domToFormat( filterEmptyNodes( afterFragment.childNodes ), format );

this.restoreContentAndSplit( before, after, blocks );
} else {
Expand Down Expand Up @@ -625,8 +624,8 @@ export class RichText extends Component {

const { format } = this.props;
this.restoreContentAndSplit(
format === 'string' ? domToString( before ) : domToElement( before ),
format === 'string' ? domToString( after ) : domToElement( after )
domToFormat( before, format ),
domToFormat( after, format )
);
}

Expand Down Expand Up @@ -698,7 +697,7 @@ export class RichText extends Component {
default:
return this.editor.dom.isEmpty( this.editor.getBody() ) ?
Copy link
Member

@aduth aduth Apr 13, 2018

Choose a reason for hiding this comment

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

Aside: Is the isEmpty check here meant to be an optimization for the easy case of generating an empty string? Is isEmpty itself actually performant? Wondering if we're actually shooting ourselves in the foot with this for the more common case that RichText is non-empty.

Copy link
Contributor Author

@youknowriad youknowriad Apr 13, 2018

Choose a reason for hiding this comment

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

It's not an optimization, it's to ensure the paragraph block is considered empty consistently. If we remove it we can end up with content like [''] which will break the default appender.

Copy link
Member

@aduth aduth Apr 13, 2018

Choose a reason for hiding this comment

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

I get the sense this has been discussed before, but if we have control over the elements we're producing, why are we keeping the empty string ?

Copy link
Contributor Author

@youknowriad youknowriad Apr 13, 2018

Choose a reason for hiding this comment

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

I can't tell honestly :) maybe @iseulde can tell more here.

Copy link
Member

@ellatrix ellatrix Apr 13, 2018

Choose a reason for hiding this comment

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

but if we have control over the elements we're producing

Do we have control over that? This one is getting the nodes from a contenteditable field which can contain all sorts combination. I've seen <p></p>,<p>''</p>, <p><br></p>, <p><br>''</p>, <p><strong></strong></p> etc., all of which are empty.

[] :
domToElement( this.editor.getBody().childNodes || [] );
domToFormat( this.editor.getBody().childNodes || [], 'element' );
Copy link
Member

@aduth aduth Apr 13, 2018

Choose a reason for hiding this comment

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

Aside: In the future, I might imagine we avoid working with the DOM as a source and instead defer to TinyMCE's getContent( { format: 'tree' } ) to convert from their tree form to the desired target format. This would avoid the need for us to maintain our own cleaning logic. At which point, domToFormat doesn't make as much sense, might just be toFormat( value, targetFormat, sourceFormat ) (maybe infer source format?).

Copy link
Contributor Author

@youknowriad youknowriad Apr 13, 2018

Choose a reason for hiding this comment

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

I tend to always prefer explicitness over implicitness. I don't really care about the name of the function but I tend to be suspicious (maybe I shoudn't) every time I hear "infer from value"

Copy link
Member

@aduth aduth Apr 13, 2018

Choose a reason for hiding this comment

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

Eh, I can see it, but it's kinda like if one had to pass a second argument to e.g. Array.from:

Array.from( new Set( [ 1, 2, 3 ] ), 'Set' );

... one could say: "Well, duh, you passed me a set, I know it's a set"

Copy link
Contributor Author

@youknowriad youknowriad Apr 13, 2018

Choose a reason for hiding this comment

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

But sometimes it's not obvious, the easy example is that the tree structure and the element structure can't be distinguished easily because both can be plain objects.

}
}

Expand Down