API to add custom formats to Editable by tg-ephox · Pull Request #3060 · WordPress/gutenberg

tg-ephox · 2017-10-19T07:32:12Z

Description

Pulled this out of Footnotes to as might be useful to block implementers.

Fixes #3059

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

New Feature

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

tg-ephox · 2017-10-19T07:36:56Z

I've added support for a simple inline style editor so that the API can be discussed. The idea is that formatters are added to Editable and registered with TinyMCE. Registration currently only happens once on editor init, but display/usage is controlled via formattingControls.

codecov · 2017-10-19T07:37:30Z

Codecov Report

Merging #3060 into master will decrease coverage by 0.03%.
The diff coverage is 8.33%.

@@            Coverage Diff            @@
##           master   #3060      +/-   ##
=========================================
- Coverage   32.23%   32.2%   -0.04%     
=========================================
  Files         216     216              
  Lines        6151    6163      +12     
  Branches     1081    1085       +4     
=========================================
+ Hits         1983    1985       +2     
- Misses       3517    3523       +6     
- Partials      651     655       +4

Impacted Files	Coverage Δ
blocks/editable/format-toolbar/index.js	`6.38% <0%> (ø)`	⬆️
blocks/editable/index.js	`10.7% <10.52%> (+0.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 653e555...e76eb2d. Read the comment docs.

youknowriad

I like this, nice work here.

Do you think the custom formatters should be registered globally and applied to all blocks using Editable or maybe with a special opt-in our opt-out prop? (which means we could use withEditorSettings to retrieve this formatters instead of a prop passed to Editable).

Keeping them as a prop has the advantage of flexibility but may add a bit of burden to support the custom formatters for block authors?

--
Also as a follow-up, I can see us providing a choice to add a formatter as a separate control (like implemented here) or as an item in a "Custom Formats" control with a dropdown.

youknowriad · 2017-10-19T09:04:17Z

blocks/editable/format-toolbar/index.js

+					...control,
+					onClick: isLink ? this.addLink : this.toggleFormat( control.format ),
+					isActive: this.isFormatActive( control.format ) || ( isLink && isAddingLink ),
+				};


Nice refactoring to merge the "link" control to the same loop 👍

youknowriad · 2017-10-19T09:05:23Z

blocks/editable/format-toolbar/index.js

-				onClick: this.toggleFormat( control.format ),
-				isActive: !! formats[ control.format ],
-			} ) );
+			.concat( customControls )


Should we concat before filtering? Thinking we could define customControls but decide to disable temporarily for some reason.

I've changed this to only pass formatters that have been filtered through formattingControls on editable.

youknowriad · 2017-10-19T09:09:08Z

blocks/editable/index.js

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


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.

youknowriad · 2017-10-19T09:09:58Z

blocks/editable/index.js

-		activeFormats.forEach( ( activeFormat ) => formats[ activeFormat ] = true );
+		const formatNames = this.props.formattingControls || FORMATS;
+
+		forEach( this.editor.formatter.matchAll( formatNames ), ( activeFormat ) => {


Should this be a map instead of forEach?

youknowriad · 2017-10-19T09:14:56Z

blocks/editable/index.js

 				formats={ this.state.formats }
 				onChange={ this.changeFormats }
 				enabledControls={ formattingControls }
+				customControls={ map( this.props.formatters, ( formatter ) => pick( formatter, [ 'format', 'title', 'icon' ] ) ) }


Is it really necessary to loop and pick only some properties?

youknowriad · 2017-10-19T09:17:38Z

blocks/editable/index.js


 	onInit() {
 		this.updateFocus();
+		this.registerCustomFormatters();


What happens if the formatters prop change, should we resync the custom formatters in TinyMCE? If it's not possible, maybe we should just guard against updating this prop in componentWillReceiveProps?

I've changed the prop from formatters to initialFormatters and added a console.error into componentWillReceiveProps, I forsee passing functions through this in future so it will be hard to determine if props have changed...

aduth · 2017-10-19T20:29:56Z

blocks/library/paragraph/index.js

 					placeholder={ placeholder || __( 'New Paragraph' ) }
+					formattingControls={ [ 'bold', 'italic', 'strikethrough', 'link', 'red' ] }
+					formatters={ [
+						createInlineStyleFormatter( 'red', 'hammer', 'Red', { color: 'red' } ),


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?

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.

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?

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.

No problems, changed this to a simple object.

tg-ephox · 2017-10-20T05:20:32Z

@youknowriad that sounds like a good idea. Should I focus on the formatter API in this issue and make another PR for global formatters?

youknowriad · 2017-10-20T08:06:02Z

blocks/editable/index.js

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


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.

youknowriad · 2017-10-20T08:09:59Z

blocks/editable/index.js

 	registerCustomFormatters() {
-		forEach( this.props.formatters, ( formatter ) => {
-			this.editor.formatter.register( formatter.format, formatter.formatter );
+		forEach( this.props.initialFormatters, ( formatter ) => {


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

youknowriad · 2017-10-20T08:12:01Z

blocks/editable/index.js

-		forEach( this.props.formatters, ( formatter ) => {
-			this.editor.formatter.register( formatter.format, formatter.formatter );
+		forEach( this.props.initialFormatters, ( formatter ) => {
+			this.editor.formatter.register( formatter.format, { ...formatter.formatter } );


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

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

youknowriad · 2017-10-20T08:12:47Z

blocks/editable/index.js

-		} );
+
+			return accFormats;
+		}, {} );


youknowriad · 2017-10-20T08:15:24Z

blocks/editable/index.js

 		this.editor.setDirty( true );
 	}

+	getToolbarCustomControls() {


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?

youknowriad · 2017-10-20T08:17:09Z

Should I focus on the formatter API in this issue and make another PR for global formatters?

@tg-ephox yep, exploring the global registering of the custom formatters could be explored separately 👍

youknowriad · 2017-10-23T07:27:29Z

blocks/library/paragraph/index.js

+						icon: 'hammer',
+						style: { color: 'red' },
+					} ]
+					}


Minor: this should probably be } ] } on the same line (like the opening side)

youknowriad

This is looking good for me. After a rebase and probably removal of the example, this should be good to go

youknowriad

Nice work, I'm approving this but please hold on merging. Let's merge after the 1.5 release.

Created simple inline style formatter

bde767c

tg-ephox added the [Status] In Progress Tracking issues with work in progress label Oct 19, 2017

tg-ephox self-assigned this Oct 19, 2017

tg-ephox requested a review from aduth October 19, 2017 07:32

tg-ephox changed the title ~~Created simple inline style formatter~~ API to add custom formats to Editable Oct 19, 2017

youknowriad reviewed Oct 19, 2017

View reviewed changes

aduth reviewed Oct 19, 2017

View reviewed changes

Changes for code review

80ab769

youknowriad reviewed Oct 20, 2017

View reviewed changes

Changes for code review

3d213e0

youknowriad reviewed Oct 23, 2017

View reviewed changes

tg-ephox added 2 commits October 24, 2017 11:50

Merge branch 'master' into try/3059-custom-formatter-api

0501aa5

Removed example style

e76eb2d

youknowriad approved these changes Oct 24, 2017

View reviewed changes

tg-ephox merged commit 21258ac into master Oct 26, 2017

tg-ephox deleted the try/3059-custom-formatter-api branch October 26, 2017 01:23

oandregal mentioned this pull request Apr 22, 2021

Spacing Support: Add margin block support with configurable sides #30608

Merged

7 tasks

Conversation

tg-ephox commented Oct 19, 2017

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

Uh oh!

tg-ephox commented Oct 19, 2017

Uh oh!

codecov bot commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tg-ephox commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tg-ephox Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov bot commented Oct 19, 2017 •

edited

Loading

tg-ephox Oct 23, 2017 •

edited

Loading