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

Rich Text: Fix Format Type Assignment During Parsing #11488

Merged
merged 2 commits into from
Nov 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 2 additions & 3 deletions packages/format-library/src/bold/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ const name = 'core/bold';
export const bold = {
name,
title: __( 'Bold' ),
match: {
tagName: 'strong',
},
tagName: 'strong',
className: null,
edit( { isActive, value, onChange } ) {
const onToggle = () => onChange( toggleFormat( value, { type: name } ) );

Expand Down
5 changes: 2 additions & 3 deletions packages/format-library/src/code/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ const name = 'core/code';
export const code = {
name,
title: __( 'Code' ),
match: {
tagName: 'code',
},
tagName: 'code',
className: null,
edit( { value, onChange } ) {
const onToggle = () => onChange( toggleFormat( value, { type: name } ) );

Expand Down
5 changes: 2 additions & 3 deletions packages/format-library/src/image/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@ export const image = {
title: __( 'Image' ),
keywords: [ __( 'photo' ), __( 'media' ) ],
object: true,
match: {
tagName: 'img',
},
tagName: 'img',
className: null,
attributes: {
className: 'class',
style: 'style',
Expand Down
5 changes: 2 additions & 3 deletions packages/format-library/src/italic/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ const name = 'core/italic';
export const italic = {
name,
title: __( 'Italic' ),
match: {
tagName: 'em',
},
tagName: 'em',
className: null,
edit( { isActive, value, onChange } ) {
const onToggle = () => onChange( toggleFormat( value, { type: name } ) );

Expand Down
5 changes: 2 additions & 3 deletions packages/format-library/src/link/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ const name = 'core/link';
export const link = {
name,
title: __( 'Link' ),
match: {
tagName: 'a',
},
tagName: 'a',
className: null,
attributes: {
url: 'href',
target: 'target',
Expand Down
5 changes: 2 additions & 3 deletions packages/format-library/src/strikethrough/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ const name = 'core/strikethrough';
export const strikethrough = {
name,
title: __( 'Strikethrough' ),
match: {
tagName: 'del',
},
tagName: 'del',
className: null,
edit( { isActive, value, onChange } ) {
const onToggle = () => onChange( toggleFormat( value, { type: name } ) );

Expand Down
27 changes: 20 additions & 7 deletions packages/rich-text/src/create.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
/**
* External dependencies
* WordPress dependencies
*/

import { find } from 'lodash';
import { select } from '@wordpress/data';

/**
* Internal dependencies
Expand All @@ -11,7 +10,6 @@ import { find } from 'lodash';
import { isEmpty } from './is-empty';
import { isFormatEqual } from './is-format-equal';
import { createElement } from './create-element';
import { getFormatTypes } from './get-format-types';
import {
LINE_SEPARATOR,
OBJECT_REPLACEMENT_CHARACTER,
Expand All @@ -36,9 +34,24 @@ function simpleFindKey( object, value ) {
}

function toFormat( { type, attributes } ) {
const formatType = find( getFormatTypes(), ( { match } ) =>
type === match.tagName
);
let formatType;

if ( attributes && attributes.class ) {
formatType = select( 'core/rich-text' ).getFormatTypeForClassName( attributes.class );
Copy link
Contributor

@youknowriad youknowriad Nov 6, 2018

Choose a reason for hiding this comment

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

Are these two places here the only places where we rely globally on the data module?
I'd like us to avoid this pattern in the future. And only use the data module in withSelect/withDispatch or provide the registry as an argument somehow (or the available formats in this case)

The issue with this pattern is that it relies on a singleton which is not great if you have the same npm module installed twice for instance.

I'm fine keeping them for now, if we already do the same elsewhere in rich-text.

Copy link
Member Author

Choose a reason for hiding this comment

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

It got the data the same way before, jut through a wrapper function. Sounds good to revisit separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just change the signature of the function to toFormat({ type, attributes }, availableFormats)? In my testing we do this in this function and in fromFormat. I'd prefer if we can avoid breaking changes here by adding this extra argument, but if you think it's a big refactoring, let's delay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where should the available formats be given? in the RichText component? And then we pass it down all the way toHTMLString( { availableFormats } ) etc.?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, yes I would prefer this or rewrite all these APIs as selectors. It does feel more impactful that I first thought so you have my 👍 for shipping as is.

Copy link
Member

Choose a reason for hiding this comment

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

This would also make writing tests easier as you don't have to include stores in tests. This issue also exists in other places. I agree that we should keep such usage to the absolute minimum. We can iterate on the codebase later.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking a bit more about it, I think that it might be better to avoid introducing more specialized selectors getFormatTypeForClassName and `getFormatTypeForBareElement. Instead, we can perform filtering using utility helpers in this file. I don't think we will need this logic anywhere else.


if ( formatType ) {
// Preserve any additional classes.
attributes.class = ` ${ attributes.class } `.replace( ` ${ formatType.className } `, ' ' ).trim();
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 happening here, can you explain? It's mostly me trying to understand why we need it. Maybe we should add an inline comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need the preserve any other classes. E.g. a plugin may add extra classes, or if the the plugin doesn't use the class attribute, user added classes should be preserved.

Copy link
Member

Choose a reason for hiding this comment

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

Right, make sense now.


if ( ! attributes.class ) {
delete attributes.class;
}
}
}

if ( ! formatType ) {
formatType = select( 'core/rich-text' ).getFormatTypeForBareElement( type );
}

if ( ! formatType ) {
return attributes ? { type, attributes } : { type };
Expand Down
49 changes: 49 additions & 0 deletions packages/rich-text/src/register-format-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,55 @@ export function registerFormatType( name, settings ) {
return;
}

if (
typeof settings.tagName !== 'string' ||
settings.tagName === ''
) {
window.console.error(
Copy link
Member

Choose a reason for hiding this comment

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

Minor: When no tag name is provided, it could show a different error.

'Format tag names must be a string.'
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR: Maybe we could leverage yup at some point for this and block registration, a validation schema seems like a good enhancement.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we need to improve error handling a lot and built some abstraction on top of it.

return;
}

if (
( typeof settings.className !== 'string' || settings.className === '' ) &&
settings.className !== null
) {
window.console.error(
'Format class names must be a string, or null to handle bare elements.'
);
return;
}

if ( ! /^[_a-zA-Z]+[a-zA-Z0-9-]*$/.test( settings.className ) ) {
window.console.error(
'A class name must begin with a letter, followed by any number of hyphens, letters, or numbers.'
);
return;
}

if ( settings.className === null ) {
const formatTypeForBareElement = select( 'core/rich-text' )
.getFormatTypeForBareElement( settings.tagName );

if ( formatTypeForBareElement ) {
window.console.error(
`Format "${ formatTypeForBareElement.name }" is already registered to handle bare tag name "${ settings.tagName }".`
);
return;
}
} else {
const formatTypeForClassName = select( 'core/rich-text' )
.getFormatTypeForClassName( settings.className );
Copy link
Member

Choose a reason for hiding this comment

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

In theory, you could allow to reuse the same class name with different tag names. We can relax it later though. Not sure about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, easier to relax than be more strict.


if ( formatTypeForClassName ) {
window.console.error(
`Format "${ formatTypeForClassName.name }" is already registered to handle class name "${ settings.className }".`
);
return;
}
}

if ( ! ( 'title' in settings ) || settings.title === '' ) {
window.console.error(
'The format "' + settings.name + '" must have a title.'
Expand Down
34 changes: 34 additions & 0 deletions packages/rich-text/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import createSelector from 'rememo';
import { find } from 'lodash';

/**
* Returns all the available format types.
Expand All @@ -28,3 +29,36 @@ export const getFormatTypes = createSelector(
export function getFormatType( state, name ) {
return state.formatTypes[ name ];
}

/**
* Gets the format type, if any, that can handle a bare element (without a
* data-format-type attribute), given the tag name of this element.
*
* @param {Object} state Data state.
* @param {string} bareElementTagName The tag name of the element to find a
* format type for.
* @return {?Object} Format type.
*/
export function getFormatTypeForBareElement( state, bareElementTagName ) {
return find( getFormatTypes( state ), ( { tagName } ) => {
return bareElementTagName === tagName;
} );
}

/**
* Gets the format type, if any, that can handle an element, given its classes.
*
* @param {Object} state Data state.
* @param {string} elementClassName The classes of the element to find a format
* type for.
* @return {?Object} Format type.
*/
export function getFormatTypeForClassName( state, elementClassName ) {
return find( getFormatTypes( state ), ( { className } ) => {
if ( className === null ) {
return false;
}

return ` ${ elementClassName } `.indexOf( ` ${ className } ` ) >= 0;
Copy link
Member

Choose a reason for hiding this comment

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

I think I get why we add wrapping spaces :)

} );
}
26 changes: 25 additions & 1 deletion packages/rich-text/src/test/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import { JSDOM } from 'jsdom';
*/
import { create } from '../create';
import { createElement } from '../create-element';
import { getSparseArrayLength, spec } from './helpers';
import { registerFormatType } from '../register-format-type';
import { unregisterFormatType } from '../unregister-format-type';
import { getSparseArrayLength, spec, specWithRegistration } from './helpers';

const { window } = new JSDOM();
const { document } = window;
Expand Down Expand Up @@ -54,6 +56,28 @@ describe( 'create', () => {
} );
} );

specWithRegistration.forEach( ( {
description,
formatName,
formatType,
html,
value: expectedValue,
Copy link
Member

Choose a reason for hiding this comment

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

I thought we could update it inside the definition,too :)

I can do it myself when working on e2e tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. It is used for other tests as well where value is the input.

} ) => {
it( description, () => {
if ( formatName ) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: for simplicity, it might be easier to add the test without formatName in their own block.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

registerFormatType( formatName, formatType );
}

const result = create( { html } );

if ( formatName ) {
unregisterFormatType( formatName );
}

expect( result ).toEqual( expectedValue );
} );
} );

it( 'should reference formats', () => {
const value = create( { html: '<em>te<strong>st</strong></em>' } );

Expand Down
77 changes: 77 additions & 0 deletions packages/rich-text/src/test/helpers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -709,3 +709,80 @@ export const spec = [
},
},
];

export const specWithRegistration = [
{
description: 'should create format by matching the class',
formatName: 'my-plugin/link',
formatType: {
title: 'Custom Link',
tagName: 'a',
className: 'custom-format',
edit() {},
},
html: '<a class="custom-format">a</a>',
value: {
formats: [ [ {
type: 'my-plugin/link',
attributes: {},
unregisteredAttributes: {},
} ] ],
text: 'a',
},
},
{
description: 'should retain class names',
formatName: 'my-plugin/link',
formatType: {
title: 'Custom Link',
tagName: 'a',
className: 'custom-format',
edit() {},
},
html: '<a class="custom-format test">a</a>',
value: {
formats: [ [ {
type: 'my-plugin/link',
attributes: {},
unregisteredAttributes: {
class: 'test',
},
} ] ],
text: 'a',
},
},
{
description: 'should create base format',
formatName: 'core/link',
formatType: {
title: 'Link',
tagName: 'a',
className: null,
edit() {},
},
html: '<a class="custom-format">a</a>',
value: {
formats: [ [ {
type: 'core/link',
attributes: {},
unregisteredAttributes: {
class: 'custom-format',
},
} ] ],
text: 'a',
},
},
{
description: 'should create fallback format',
html: '<a class="custom-format">a</a>',
value: {
formats: [ [ {
type: 'a',
attributes: {
class: 'custom-format',
},
} ] ],
text: 'a',
},
},
];
Loading