Text formatting UI #460

ellatrix · 2017-04-20T09:55:56Z

Still needs a styling fix and a look into Range errors, but wanted to push it here to see what others think.

youknowriad · 2017-04-20T10:52:11Z

Tested and is working pretty well but I have several questions/remarks here.

How do you think we would handle multi-editable blocks like quote?
Something that bothers me with this approach is its "imperativeness". React is declarative by default, which could mean props for a component are description of a state or an option but not an imperative command. which brings me to the requestFormat prop which if I understand correctly is a set of commands to trigger inside the Editable component and right after the command is triggered we call a callback onPathChange that resets these commands.

Could we pass the formats state instead and compare with the previous passed state in componentWillReceiveProps to trigger TinyMCE's commands? This would allow us to pass scalar values as well (think Link Control)

Also, you're extracting the formats values (toggled or not) by passing a path to the VisualBlock component. I think the VisualBlock should not know about paths and stuff like that (these are TinyMCE specific). Could we just move this behavior inside the Editable component and directly pass the formats state as a paramter to onFormatChange prop or similar (as opposed to the current onPathChange prop)

ellatrix · 2017-04-20T13:19:52Z

How do you think we would handle multi-editable blocks like quote?

As discussed earlier in Slack, I think we should mark some areas as the main text area or a field that needs controls repositioned. We'll need a way to distinguish between the two.

Something that bothers me with this approach is its "imperativeness". React is declarative by default, which could mean props for a component are description of a state or an option but not an imperative command. which brings me to the requestFormat prop which if I understand correctly is a set of commands to trigger inside the Editable component and right after the command is triggered we call a callback onPathChange that resets these commands.

Suggestions welcome! If we use formats, how do we distinguish between a path change and a command?

youknowriad · 2017-04-20T13:23:09Z

Suggestions welcome! If we use formats, how do we distinguish between a path change and a command?

Yep, it's probably a bad name, the most important for me is having an object containing all the inline controls data.

const inlineState = {
  bold: true,
  italic: false,
  link: { url: 'http://google.com', target: '_blank' }
}

ellatrix · 2017-04-20T13:26:10Z

Sure, that's the same but a different name. What if the state changes by changing the selection, not by clicking the button. How does Editable know what happened?

youknowriad · 2017-04-20T13:28:09Z

that's the onFormatChange I was talking about above (the name could be different). computing the new inlineState would be done inside the Editable

ellatrix · 2017-04-20T14:02:05Z

@youknowriad Changed some things. I'll try to get rid of requestFormat but can't see immediately how to.

youknowriad · 2017-04-20T14:05:22Z

I'll try to get rid of requestFormat but can't see immediately how to.

Can we do this by passing this.state.formats instead of this.state.requestFormat as a prop? And maybe using this.editor.executeCommand once the value of a format changes?

ellatrix · 2017-04-20T14:06:55Z

Yeah, we can cache the path and compare it, so we're not executing the commands on selection changes.

youknowriad · 2017-04-20T14:08:11Z

Yeah, we can cache the path and compare it, so we're not executing the commands on selection changes.

I was thinking of comparing this.props.formats with nextProps.formats in componentWillReceiveProps.

ellatrix · 2017-04-20T14:22:55Z

Like this? Any other thoughts?

youknowriad · 2017-04-20T14:29:59Z

blocks/components/editable/index.js

+
+		forEach( nextProps.formats, ( state, format ) => {
+			if ( state !== this.props.formats[ format ] ) {
+				this.editor.formatter.toggle( formatMap[ format ] );


Thanks for the changes @iseulde. This starts to look good for me. But I think we should not use toggle here. I'm seeing some bugs while the format is changed when I click in different places of the Editable. I suspect this is caused by the toggle because we're not taking into account the value of this.props.formats[ format ] whether it's true or false. I don't know maybe something like this.editor.execCommand( formatMap[ format ], true, this.props.formats[ format ] ) would fix it?

Or if toggle is the right way to do it, we should probably check whether the current path is already equivalent to the this.props.formats[ format ] value before calling toggle

Does the new state check make sense?

(And explicitly calling apply or remove.)

youknowriad · 2017-04-20T14:33:25Z

blocks/components/editable/index.js

+		const formatMap = {
+			strong: 'bold',
+			em: 'italic'
+		};


I wonder if we should do the mapping before calling the onFormatChange instead

Yep, we should.

jasmussen · 2017-04-20T15:44:31Z

Works great!

Can we get link and strikethrough in also?

ellatrix · 2017-04-20T15:48:32Z

Sure strikethrough is the same. Link will need more UI. Maybe best a separate PR.

ellatrix · 2017-04-20T15:54:19Z

We can also probably get rid for the format map by checking TinyMCE settings for that...

youknowriad · 2017-04-20T16:43:21Z

blocks/components/editable/index.js

@@ -127,6 +146,22 @@ export default class Editable extends wp.element.Component {
 		}
 	}

+	componentWillReceiveProps( nextProps ) {
+		forEach( nextProps.formats, ( state, format ) => {
+			const currentState = this.formats[ format ] || false;


I would prefer if we find a way to avoid the cache this.formats but it's ok for now.

Is this.formats here any different than this.props.formats? Like Riad, I also instinctively cringe at the sight of instance variables, though they're sometimes justified.

Yes, It can be different than this.props.formats right after we trigger the onFormatChange callback.

Any suggestions here are welcome.

youknowriad · 2017-04-20T16:45:05Z

blocks/library/text/index.js

@@ -47,7 +47,25 @@ registerBlock( 'core/text', {
 		}
 	],

-	edit( { attributes, setAttributes, insertBlockAfter } ) {
+	formatting: [


Just pinging @aduth maybe he has some thoughts on the block API part of this PR. The formatting attribute here and the two props onFormatChange and formats

aduth · 2017-04-20T19:59:04Z

blocks/components/editable/index.js

@@ -47,6 +54,17 @@ export default class Editable extends wp.element.Component {
 		editor.on( 'focusout', this.onChange );
 		editor.on( 'NewBlock', this.onNewBlock );
 		editor.on( 'focusin', this.onFocus );
+
+		if ( this.props.onFormatChange ) {
+			editor.on( 'nodechange', ( { parents } ) => {


Can we move the callback logic to a separate named function on the class? Just in an effort to avoid a sprawling setup method.

aduth · 2017-04-20T20:05:54Z

blocks/components/editable/index.js

+					formatMap[ node.nodeName.toLowerCase() ]
+				) );
+
+				this.formats = zipObject( path, path.map( () => true ) );


Suggestion for O(4n) to O(n) optimization:

const formats = parents.reduce( ( memo, node ) => { const tagName = node.nodeName.toLowerCase(); if ( formatMap.hasOwnProperty( tagName ) ) { memo[ formatMap[ tagName ] ] = true; } return memo; }, {} );

aduth · 2017-04-20T20:11:50Z

blocks/library/text/index.js

+		}
+	],
+
+	edit( { attributes, setAttributes, insertBlockAfter, focus, setFocus, onFormatChange, formats } ) {


Not specific to this pull request, but I'm concerned about how many props an implementer is expected to be aware of just to use an Editable component. Previously I'd suggested considering context as a transparent pass-through. I don't really know whether it's a perfect solution, but we'll need to think of something. Maybe also something in the global state instead of VisualBlock's state.

Same here, also since these are probably things the block implementor doesn't need to know anything about.

aduth · 2017-04-20T20:13:25Z

blocks/library/text/index.js

@@ -47,7 +47,25 @@ registerBlock( 'core/text', {
 		}
 	],

-	edit( { attributes, setAttributes, insertBlockAfter, focus, setFocus } ) {
+	formatting: [


In API docs I'd remarked that "Inline controls for Editable elements are identical for every block and cannot be modified." The thinking being that it'd be quite rare for an editable to not need these standard formatting controls, even to the point that we'd want to discourage it for the break in consistency. Do you have any feelings on this? Or could we at least have a standard set and make the intent an opt-out instead of opt-in?

Also, if we're introducing changes to the block API, we should probably document them at the same time. Specifically thinking about available format options.

I agree with this. I don't think a plugin should be able to change these (would love to see examples where they should). From earlier conversations I thought we wanted it like this.

aduth · 2017-04-20T20:14:08Z

blocks/components/editable/index.js

+
+			if ( state !== currentState ) {
+				if ( state ) {
+					this.editor.focus();


Minor: If the focus applies first to both if and else, might as well move it above the condition (single occurrence).

aduth · 2017-04-20T20:18:47Z

editor/modes/visual-editor/block.js

 	}

 	bindBlockNode( node ) {
 		this.node = node;
 	}

+	onFormatChange( formats ) {
+		if ( ! isEqual( formats, this.state.formats ) ) {


Why would we allow onFormatChange to be invoked if a change actually hasn't occurred? Or in any case, why would we optimize for it? Thinking we can drop the isEqual check altogether.

aduth · 2017-04-20T20:20:01Z

editor/modes/visual-editor/block.js

 	}

 	bindBlockNode( node ) {
 		this.node = node;
 	}

+	onFormatChange( formats ) {
+		if ( ! isEqual( formats, this.state.formats ) ) {
+			this.setState( ( prevState ) => {


Curious about the choice of callback signature. Why not:

onFormatChange( formats ) { this.setState( { formats } }; }

aduth · 2017-04-20T20:21:34Z

editor/modes/visual-editor/block.js

+	}
+
+	toggleFormat( format ) {
+		this.setState( ( prevState ) => {


Similar to above, maybe more concise:

const { formats } = this.state; this.setState( { formats: { ...formats, [ format ]: ! formats[ format ] } } );

aduth · 2017-04-20T20:24:10Z

editor/modes/visual-editor/block.js

+							controls={ settings.formatting.map( ( control ) => ( {
+								...control,
+								onClick: () => this.toggleFormat( control.format ),
+								isActive: () => !! this.state.formats[ control.format ]


Hmm, reconsidering whether isActive really makes sense as a function. We can save this for a separate pull request, but seems like a boolean would be a better substitute

aduth · 2017-04-20T20:25:31Z

blocks/components/editable/index.js

@@ -127,6 +146,22 @@ export default class Editable extends wp.element.Component {
 		}
 	}

+	componentWillReceiveProps( nextProps ) {
+		forEach( nextProps.formats, ( state, format ) => {
+			const currentState = this.formats[ format ] || false;


Is this.formats here any different than this.props.formats? Like Riad, I also instinctively cringe at the sight of instance variables, though they're sometimes justified.

youknowriad · 2017-04-24T13:46:53Z

editor/modes/visual-editor/block.js

 	}

 	bindBlockNode( node ) {
 		this.node = node;
 	}

+	onFormatChange( formats ) {
+		if ( ! this.state.hasEditable ) {


This feels a bit like a hack :) but in the same time I can't find a better alternative

An alternative would be an extra setting for it, but not sure if that's any better.

youknowriad

Really good job on this @iseulde. This is good to be merged for me 👍

aduth · 2017-04-24T15:37:45Z

I'm still trying to narrow in on what's causing it, but specific to this branch I don't have a text cursor (I-beam) when hovering the text block and can't select any text.

youknowriad · 2017-04-24T15:41:45Z

@aduth Probably caused by this #494 A rebase should fix it

aduth

Sure enough, that was it @youknowriad . Works well after a local rebase. LGTM 👍

ellatrix requested a review from youknowriad April 20, 2017 10:04

ellatrix force-pushed the add/text-formatting branch from 2a1b0c4 to fd4e667 Compare April 20, 2017 13:45

youknowriad reviewed Apr 20, 2017

View reviewed changes

ellatrix self-assigned this Apr 20, 2017

youknowriad reviewed Apr 20, 2017

View reviewed changes

ellatrix force-pushed the add/text-formatting branch from 241e2b6 to 869f34b Compare April 20, 2017 17:01

aduth reviewed Apr 20, 2017

View reviewed changes

ellatrix added 7 commits April 24, 2017 15:25

Text formatting UI

395ad1b

Move nodeChange callback to separate function

c48a996

Remove double

6546b7c

Remove unnecessary logic in and

4e00fec

Fix linting error

76e3da6

Move formatting control settings to

6b61f8c

Optimise in . Props @aduth

a8c964d

ellatrix force-pushed the add/text-formatting branch from 0b8cf56 to a8c964d Compare April 24, 2017 13:31

youknowriad reviewed Apr 24, 2017

View reviewed changes

youknowriad approved these changes Apr 24, 2017

View reviewed changes

aduth approved these changes Apr 24, 2017

View reviewed changes

ellatrix merged commit 2ef33e2 into master Apr 24, 2017

ellatrix deleted the add/text-formatting branch April 24, 2017 15:48

This was referenced Apr 24, 2017

Assign control isActive as boolean #500

Merged

Try Slot & Fill pattern for rendering Editable formatting controls #507

Merged

jasmussen mentioned this pull request Apr 25, 2017

First milestone: Prototype Parity #462

Closed

15 tasks

ellatrix added this to the Prototype Parity milestone Apr 26, 2017

Verdinjoshua26 mentioned this pull request Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#57

Open

Verdinjoshua26 mentioned this pull request Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#59

Open

Verdinjoshua26 mentioned this pull request Oct 1, 2023

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#87

Open

Text formatting UI #460

Text formatting UI #460

Conversation

ellatrix commented Apr 20, 2017

youknowriad commented Apr 20, 2017

ellatrix commented Apr 20, 2017

youknowriad commented Apr 20, 2017

ellatrix commented Apr 20, 2017

youknowriad commented Apr 20, 2017

ellatrix commented Apr 20, 2017

youknowriad commented Apr 20, 2017

ellatrix commented Apr 20, 2017

youknowriad commented Apr 20, 2017

ellatrix commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Apr 20, 2017

ellatrix commented Apr 20, 2017

ellatrix commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented Apr 24, 2017

youknowriad commented Apr 24, 2017 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

youknowriad Apr 20, 2017 •

edited

Loading

youknowriad commented Apr 24, 2017 •

edited

Loading