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.

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

Jump to bottom

API to add custom formats to Editable #3060

Merged
merged 5 commits into from
Oct 26, 2017
Merged

API to add custom formats to Editable #3060

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bde767c
Created simple inline style formatter
tg-ephox Oct 19, 2017
80ab769
Changes for code review
tg-ephox Oct 20, 2017
3d213e0
Changes for code review
tg-ephox Oct 23, 2017
0501aa5
Merge branch 'master' into try/3059-custom-formatter-api
tg-ephox Oct 24, 2017
e76eb2d
Removed example style
tg-ephox Oct 24, 2017
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
54 changes: 37 additions & 17 deletions blocks/editable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import {
find,
defer,
noop,
map,
pick,
} from 'lodash';
import { nodeListToReact } from 'dom-react';
import 'element-closest';
Expand Down Expand Up @@ -60,15 +58,17 @@ function isLinkBoundary( fragment ) {
fragment.childNodes[ 0 ].text[ 0 ] === '\uFEFF';
}

function getLinkProperties( element ) {
return { value: element.getAttribute( 'href' ) || '', target: element.getAttribute( 'target' ) || '', node: element };
}

function getFormatProperties( formatName, parents ) {
return formatName === 'link' ? getLinkProperties( find( parents, node => node.nodeName.toLowerCase() === 'a' ) ) : {};
switch ( formatName ) {
case 'link' :
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add scoping here:

case 'link': {
     // code here
  }

Switch cases do not scope let/const variables by default, it could create confusion with other "cases", it's a good practice to scope the "case" whenever we declare a variable there.

const anchor = find( parents, node => node.nodeName.toLowerCase() === 'a' );
return !! anchor ? { value: anchor.getAttribute( 'href' ) || '', target: anchor.getAttribute( 'target' ) || '', node: anchor } : {};
default:
return {};
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor and personal preference: I would have used a single function and a switch on formatName. It makes it easy to extend if needed but it's not very important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


const FORMATS = [ 'bold', 'italic', 'strikethrough', 'link' ];
const DEFAULT_FORMATS = [ 'bold', 'italic', 'strikethrough', 'link' ];

export default class Editable extends Component {
constructor( props ) {
Expand Down Expand Up @@ -159,8 +159,8 @@ export default class Editable extends Component {
}

registerCustomFormatters() {
forEach( this.props.formatters, ( formatter ) => {
this.editor.formatter.register( formatter.format, formatter.formatter );
forEach( this.props.initialFormatters, ( formatter ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Actually, I'd prefer leaving the prop as "formatters", coupled with the console.error but I'm fine with both

this.editor.formatter.register( formatter.format, { ...formatter.formatter } );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a new instance? Also the formatter.formatter bothers me a bit, maybe we could deconstruct instead?

Copy link
Contributor Author

@tg-ephox tg-ephox Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We do in this instance as Tiny mutates the formatter and then we can't compare this.props.formatters with nextProps.formatters in componentWillReceiveProps.

} );
}

Expand Down Expand Up @@ -492,15 +492,15 @@ export default class Editable extends Component {
}

onNodeChange( { parents } ) {
const formats = {};
const formatNames = this.props.formattingControls || FORMATS;

forEach( this.editor.formatter.matchAll( formatNames ), ( activeFormat ) => {
formats[ activeFormat ] = {
const formatNames = this.props.formattingControls;
const formats = this.editor.formatter.matchAll( formatNames ).reduce( ( accFormats, activeFormat ) => {
accFormats[ activeFormat ] = {
isActive: true,
...getFormatProperties( activeFormat, parents ),
};
} );

return accFormats;
}, {} );
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


const focusPosition = this.getFocusPosition();
this.setState( { formats, focusPosition, selectedNodeId: this.state.selectedNodeId + 1 } );
Expand Down Expand Up @@ -570,6 +570,15 @@ export default class Editable extends Component {
}
}

componentWillReceiveProps( nextProps ) {
if ( 'development' === process.env.NODE_ENV ) {
if ( ! isEqual( this.props.initialFormatters, nextProps.initialFormatters ) ) {
// eslint-disable-next-line no-console
console.error( 'Formatters passed via `initialFormatters` will only be registered once. Formatters can be enabled/disabled via the `formattingControls` prop.' );
}
}
}

isFormatActive( format ) {
return this.state.formats[ format ] && this.state.formats[ format ].isActive;
}
Expand Down Expand Up @@ -612,6 +621,12 @@ export default class Editable extends Component {
this.editor.setDirty( true );
}

getToolbarCustomControls() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning for moving this filter to Editable? Since there's a filter already done in the formatting toolbar, I'd have expected it to be done there?

const { initialFormatters, formattingControls } = this.props;
return initialFormatters
.filter( f => formattingControls.indexOf( f.format ) > -1 );
}

render() {
const {
tagName: Tagname = 'div',
Expand Down Expand Up @@ -641,7 +656,7 @@ export default class Editable extends Component {
formats={ this.state.formats }
onChange={ this.changeFormats }
enabledControls={ formattingControls }
customControls={ map( this.props.formatters, ( formatter ) => pick( formatter, [ 'format', 'title', 'icon' ] ) ) }
customControls={ this.getToolbarCustomControls() }
/>
);

Expand Down Expand Up @@ -684,3 +699,8 @@ export default class Editable extends Component {
Editable.contextTypes = {
onUndo: noop,
};

Editable.defaultProps = {
formattingControls: DEFAULT_FORMATS,
initialFormatters: [],
};
2 changes: 1 addition & 1 deletion blocks/library/paragraph/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ registerBlockType( 'core/paragraph', {
onReplace={ onReplace }
placeholder={ placeholder || __( 'New Paragraph' ) }
formattingControls={ [ 'bold', 'italic', 'strikethrough', 'link', 'red' ] }
formatters={ [
initialFormatters={ [
createInlineStyleFormatter( 'red', 'hammer', 'Red', { color: 'red' } ),
Copy link
Member

Choose a reason for hiding this comment

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

Without looking at the function definition, it is difficult to know at a glance what these arguments represent. Could we not just pass this as an array of objects? Why do we need createInlineStyleFormatter at all if we can provide a plain object representation that satisfies the need?

Copy link
Member

Choose a reason for hiding this comment

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

Or if formatters need to be keyed by a name (which we're passing as an argument), then an object of key => setting pairs.

Could we generate a name on behalf of the implementer, based on the other settings provided? Something like a json-stable-stringify on the other options to create a unique and deterministic key. Might be overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can pass the args as an object? I foresee the formatting object getting a lot more complex like in

gutenberg/blocks/library/paragraph/index.js

Line 109 in ece64ee

this.footnoteFormatter = {
to deal with formats that involve other settings, so thought it might be better to always use a function to create a formatter?

Copy link
Member

Choose a reason for hiding this comment

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

If the formatting object becomes complex, how does the function remedy this? If it's the case that it generates many of the values of the objects based on the arguments, then I expect we could still do similar by generating those values based on the incoming formatters prop objects when they come time to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problems, changed this to a simple object.

] }
/>
Expand Down