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
Fix splitting
  • Loading branch information
@ellatrix
ellatrix committed Apr 16, 2018
commit db265218feaffc8aef3e2e4bda6b29fedb19b7bd
20 changes: 14 additions & 6 deletions blocks/rich-text/format.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { omitBy, map } from 'lodash';
import { omitBy } from 'lodash';
import { nodeListToReact } from 'dom-react';

/**
Expand Down Expand Up @@ -76,26 +76,34 @@ export function domToElement( value ) {
/**
* Transforms an array of DOM Elements to their corresponding HTML string output.
*
* @param {Array} value DOM Elements.
* @param {Array} value DOM Elements.
* @param {Editor} editor TinyMCE editor instance.
*
* @return {string} HTML.
*/
export function domToString( value ) {
return map( value, element => element.outerHTML ).join( '' );
export function domToString( value, editor ) {
const doc = document.implementation.createHTMLDocument( '' );

Array.from( value ).forEach( ( child ) => {
doc.body.appendChild( child );
} );

return editor ? editor.serializer.serialize( doc.body ) : doc.body.innerHTML;
}

/**
* Transforms an array of DOM Elements to the given format.
*
* @param {Array} value DOM Elements.
* @param {string} format Output format (string or element)
* @param {Editor} editor TinyMCE editor instance.
*
* @return {*} Output.
*/
export function domToFormat( value, format ) {
export function domToFormat( value, format, editor ) {
switch ( format ) {
case 'string':
return domToString( value );
return domToString( value, editor );
default:
return domToElement( value );
}
Expand Down
14 changes: 7 additions & 7 deletions blocks/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,8 +476,8 @@ export class RichText extends Component {
const afterNodes = childNodes.slice( index + 1 );

const { format } = this.props;
const before = domToFormat( beforeNodes, format );
const after = domToFormat( afterNodes, format );
const before = domToFormat( beforeNodes, format, this.editor );
const after = domToFormat( afterNodes, format, this.editor );

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

const { format } = this.props;
const before = domToFormat( beforeFragment.childNodes, format );
const after = domToFormat( filterEmptyNodes( afterFragment.childNodes ), format );
const before = domToFormat( beforeFragment.childNodes, format, this.editor );
const after = domToFormat( filterEmptyNodes( afterFragment.childNodes ), format, this.editor );

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

const { format } = this.props;
this.restoreContentAndSplit(
domToFormat( before, format ),
domToFormat( after, format )
domToFormat( before, format, this.editor ),
domToFormat( after, format, this.editor )
);
}

Expand Down Expand Up @@ -691,7 +691,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.

[] :
domToFormat( this.editor.getBody().childNodes || [], 'element' );
domToFormat( this.editor.getBody().childNodes || [], 'element', this.editor );
}
}

Expand Down