Editable: Adding an inline link control to the format toolbar #524

youknowriad · 2017-04-27T12:25:23Z

This PR adds the link control to the formatting toolbar.

Testing instructions

Try adding links and navigating inside an Editable.

youknowriad · 2017-04-27T12:25:50Z

This is not polished yet, but I wanted to have some feedback.

jasmussen · 2017-04-27T12:37:08Z

Nice work! I do see that it's still in progress, but this gets us much closer to the current link behavior, and it looks good!

aduth · 2017-04-27T12:58:47Z

An issue I'm observing:

Click into text block at a word
Click Link button in toolbar
Move focus into link URL field
Click Trash icon

Expected: Link to be removed
Actual: Link not removed, but modal is dismissed

Guessing something to do with focus being changed. Though strangely it does work with the same steps if in step 1 you highlight text instead of simply clicking into it.

aduth · 2017-04-27T12:54:05Z

blocks/components/editable/format-toolbar.js

+/**
+ * Internal dependencies
+ */
+import './style.scss';


I don't think we need to import the styles here if we're already doing so in index.js.

aduth · 2017-04-27T12:59:40Z

blocks/components/editable/format-toolbar.js

+ // as we're doing here; instead, we should consider a common components path.
+import IconButton from '../../../editor/components/icon-button';
+
+const formattingControls = [


Idea: Do you think there's value in a naming convention for constants. The uh, well... "screaming snake case" convention of FORMATTING_CONTROLS?

There's precedent with the PHP styling guidelines, but not a similar mention in JavaScript (perhaps worth championing):

Constants should be in all upper-case with underscores separating words:

define( 'DOING_AJAX', true );

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions

aduth · 2017-04-27T13:02:43Z

blocks/components/editable/index.js

-		} else {
-			this.editor.formatter.apply( format );
-		}
+		Object.keys( formats ).forEach( format => {


Could use Lodash's forEach which works on objects and would optimize from O(2n) to O(n):

forEach( formats, ( formatValue, format ) => {

aduth · 2017-04-27T13:03:39Z

blocks/components/editable/index.js

-							onClick: () => this.toggleFormat( control.format ),
-							isActive: this.isFormatActive( control.format )
-						} ) ) } />
+					<FormatToolbar focusPosition={ this.state.focusPosition } formats={ this.state.formats } onChange={ this.changeFormats } />


Might want to split these props onto multiple lines to avoid the long line length.

Instead of calculating the position, would it be possible to do some absolute positioning within a relative control parent for the overlay?

Something like: https://codepen.io/aduth/pen/LyxJEb

Oh, I see now we're trying to position the overlay atop the selected text. I suppose this works too, just maybe a little unexpected to have to move cursor to toolbar to add link, only to have to move it back to text to configure the link. I do see this is how it works in the current editor though.

Maybe something for @jasmussen to weigh in on with regards to the placement of the link dialog. Should it be next to the selected text, or next to the now-active link toolbar control?

I do like this link design (though the specific buttons are a bit quickly put together):

@iseulde should be in the loop on the link work we're doing here, because IMO links work better in the current WordPress editor than anywhere else, and she worked a lot on them. Having the dialog under as opposed to over was for a reason, though I can't recall which.

Also, here's an admittedly suboptimal design for when you are linking inside captions:

Still thinking about that one, but it's at least one argument for having links under.

Yeah we put in under the link because we kept in mind that we might want to do something different above. There might be more reasons I don't remember though.

Oh, I see now we're trying to position the overlay atop the selected text. I suppose this works too, just maybe a little unexpected to have to move cursor to toolbar to add link, only to have to move it back to text to configure the link. I do see this is how it works in the current editor though.

We were planning to look into putting the formatting tools inline which would solve this. If we keep them on top, it might be worth exploring other solutions though. There's also cmd+K and pasting links over words :)

aduth · 2017-04-27T13:12:35Z

blocks/components/editable/index.js

-		}
+		Object.keys( formats ).forEach( format => {
+			const formatValue = formats[ format ];
+			if ( format === 'link' ) {


Based on the distinctly unique treatment between links and other formatting, I'm wondering if they ought to be handled separately. Maybe not; I do like the single onChange prop applied to the toolbar component.

aduth · 2017-04-27T13:22:33Z

blocks/components/editable/format-toolbar.js

+		const { formats, focusPosition } = this.props;
+		const linkStyle = focusPosition
+			? { position: 'absolute', ...focusPosition }
+			: {};


Minor, and doesn't affect this case, but I tend to default to null as the empty value, since it can be strictly compared, in case what you're applying it to happens to be a pure component.

Demo: https://codepen.io/aduth/pen/JNEaRb

aduth · 2017-04-27T13:24:17Z

blocks/components/editable/format-toolbar.js

+						style={ linkStyle }
+						onSubmit={ this.submitLink }>
+						<input type="url" value={ this.state.linkValue } onChange={ this.updateLinkValue } />
+						<IconButton icon="editor-break" onClick={ this.submitLink } />


I wonder if we ought to be able to make IconButton behave as a submit. Actually, based on the additional props behavior, we might just be able to pass type="submit".

aduth · 2017-04-27T13:26:53Z

blocks/components/editable/index.js

+		Object.keys( formats ).forEach( format => {
+			const formatValue = formats[ format ];
+			if ( format === 'link' ) {
+				if ( formatValue !== undefined ) {


To this and the above TODO, what's the use case for an empty link? Should we just check general truthiness here, including treating an empty string as an unlink intent?

The use-case is showing the link modal at the right position when we toggle the link control in the toolbar.

To show the link modal we could rely on a state boolean but we need a dom element around our selection to compute the right position for the link modal. So my idea is to insert an empty link when we hit the link control.

aduth · 2017-04-27T13:28:11Z

blocks/components/editable/format-toolbar.js

+		this.addLink = this.addLink.bind( this );
+		this.dropLink = this.dropLink.bind( this );
+		this.submitLink = this.submitLink.bind( this );
+		this.updateLinkValue = this.updateLinkValue.bind( this );


The instance property initializers feature is growing on me 😅

youknowriad · 2017-04-28T08:45:26Z

blocks/components/editable/format-toolbar.js

+	addLink() {
+		if ( ! this.props.formats.link ) {
+			// TODO find a way to add an empty link to TinyMCE
+			this.props.onChange( { link: 'http://wordpress.org' } );


Here's where I need an empty link @iseulde. This allows adding a "node" to show the link modal at the right place

In core we put _wp_link_placeholder as the href and then make sure it's stripped everywhere appropriate. :)

ellatrix · 2017-04-28T10:46:52Z

Going to have a look in a moment.

ellatrix · 2017-04-28T11:31:36Z

One thing that seems to be missing here is a state for just viewing the link. So when you put the caret inside a link, you can see what the link is (also characters would be encoded), click on it to go to the page, edit it, and remove it. When you click the edit button you have an input field with the decoded URL, a save button, and an advanced button. We were thinking of getting rid of the big link modal entirely and have a dropdown for options or something. None by default, but a place where plugins can extend.

ellatrix · 2017-04-28T11:34:34Z

Here's an example of on encoded ink:

The one in the input field would be decoded which would be a pain to read.

youknowriad · 2017-04-28T12:35:00Z

@iseulde @jasmussen

I've added the "view" state in 17314f3 and the behaviour here is starting to be solid.
This needs some design love
I think we should see if we can get some help from the TinyMCE team to allow adding an empty link.

aduth · 2017-04-28T15:43:44Z

I think we should see if we can get some help from the TinyMCE team to allow adding an empty link.

Instead of using the command which validates against emptiness, we could take a more direct approach with formatter.apply, which has the benefit of perhaps letting us be more consistent with the other formatting we apply (since we're already using formatter.apply for bold, italic, strike).

https://github.com/tinymce/tinymce/blob/8695816/src/core/src/main/js/EditorCommands.js#L545

youknowriad · 2017-04-28T15:55:51Z

@aduth Good idea but unfortunately it's not working. I'm having this error.

aduth · 2017-04-28T16:04:17Z

Based on the implementation, it won't be as easy to use as the bold / italic / strike formatting, since it requires the anchor node. Are you passing that?

I'd also suggest dropping the .min suffix on the TinyMCE dependency and checking where the error's occurring in the unminified code. Aside: We should probably load this unminified version when SCRIPT_DEBUG constant is enabled.

youknowriad · 2017-05-01T09:35:25Z

@aduth I was able to fix the empty link issue by disabling the URL Converter.
I've also polished the modal style a bit.

aduth · 2017-05-01T14:51:06Z

blocks/components/editable/format-toolbar.js

+			this.props.onChange( { link: { value: '' } } );
+
+			// Debounce the call to avoid the reset in willReceiveProps
+			setTimeout( () => this.setState( { isEditingLink: true } ) );


Any time we use setTimeout or debounce, we can't be guaranteed that the component instance is still rendered when the callback is called. Therefore we must assign the timeout reference (setTimeout return value) into the instance and call clearTimeout in componentWillUnmount. Otherwise you'll encounter errors if the component is unmounted.

As a general note, this is one of the reasons I try to discourage setTimeout in a component (or any asynchronous action for that matter); because it's very difficult to track what's occurred in the time between when the timeout is declared and the callback is invoked.

Yes, and I always forget 😕. I don't have a better alternative here.

Should be ok now

aduth · 2017-05-01T14:53:30Z

blocks/components/editable/format-toolbar.js

+
+		return (
+			<div className="editable-format-toolbar">
+				<ul className="editor-toolbar">


Can't we use Toolbar component here? At the very least, I'm not particularly comfortable with inheriting styles from the editor-toolbar class because it's hard to track them back to being used in this component.

If we use Toolbar here we'll need to update its implementation, the same way I did here #505 (comment) and I guess you don't like this implementation too.

Maybe not 🤔 I'll try.

The behaviour was different from my previous PR. thus, I was able to use the Toolbar component 🎉

aduth · 2017-05-01T14:55:58Z

blocks/components/editable/index.js

+		} );
+
+		this.setState( {
+			formats: merge( this.state.formats, formats )


You're mutating this.state directly here. Consider passing empty object {} as first argument instead.

Never mutate this.state directly, as calling setState() afterwards may replace the mutation you made. Treat this.state as if it were immutable.

https://facebook.github.io/react/docs/react-component.html#state

Note: This method mutates object.

https://lodash.com/docs/4.17.4#merge

aduth · 2017-05-01T14:57:13Z

blocks/components/editable/index.js

+					if ( ! anchor ) {
+						this.editor.formatter.remove( 'link' );
+					}
+					this.editor.formatter.apply( 'link', { href: formatValue.value }, anchor );


Should this occur if ! anchor condition above passes and the link is removed? Seems like this ought to be:

if ( anchor ) { // apply link } else { // remove link }

No, this is not the desired behaviour here.

Actually I'm not sure this is very important but I just mimic the insertLink command here. I think this might be useful if we try to add multiple links to the same node.

Polish the link modal

youknowriad · 2017-05-02T08:53:08Z

I'm planning to merge this today, let me know if you have any other concern.

- clear the `setTimeout` on unmount - don't mutate the state object

aduth · 2017-05-02T18:40:44Z

blocks/components/editable/format-toolbar.js

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


If we're not using FORMATTING_CONTROLS for anything else, and we're always concatenating, why not just include the link control in the constant set?

They don't have the same isActive and onClick prop. Do you prefer a ternary or something?

Ah, I think I overlooked how we're mapping to add in instance-bound handlers. This should be fine as-is.

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 27, 2017

youknowriad self-assigned this Apr 27, 2017

youknowriad requested a review from ellatrix April 27, 2017 12:25

youknowriad requested a review from jasmussen April 27, 2017 12:25

youknowriad mentioned this pull request Apr 27, 2017

Editable: Adding the link control #505

Closed

youknowriad force-pushed the add/link-control-inline branch from e2c1f8a to a25fe23 Compare April 27, 2017 12:33

aduth added the [Status] In Progress Tracking issues with work in progress label Apr 27, 2017

aduth reviewed Apr 27, 2017

View reviewed changes

youknowriad removed their assignment Apr 27, 2017

youknowriad force-pushed the add/link-control-inline branch from b3b3741 to 833b944 Compare April 27, 2017 14:02

aduth mentioned this pull request Apr 27, 2017

Add formatting toolbar to text blocks #452

Closed

youknowriad commented Apr 28, 2017

View reviewed changes

youknowriad removed the [Status] In Progress Tracking issues with work in progress label Apr 28, 2017

youknowriad force-pushed the add/link-control-inline branch from 17314f3 to 3d0fbee Compare April 28, 2017 13:14

youknowriad force-pushed the add/link-control-inline branch from 93d0036 to 009849c Compare May 1, 2017 08:47

aduth reviewed May 1, 2017

View reviewed changes

youknowriad force-pushed the add/link-control-inline branch 2 times, most recently from 74bddb9 to 4a6c207 Compare May 1, 2017 15:26

youknowriad self-assigned this May 1, 2017

youknowriad added 5 commits May 2, 2017 09:48

Editable: Adding an inline link control to the format toolbar

28b7e06

Format Toolbar: Small style changes

d6a006c

Link Control: Adding the view link state

609f2a5

Link control: Trigger the edit state when inserting an new link

328b212

Format Controls: Fixing the empty link insertion

31a5778

Polish the link modal

youknowriad force-pushed the add/link-control-inline branch from 4a6c207 to 25016bc Compare May 2, 2017 08:52

Format Controls: Leverage the Toolbar component

910d2dc

- clear the `setTimeout` on unmount - don't mutate the state object

youknowriad force-pushed the add/link-control-inline branch from 25016bc to 910d2dc Compare May 2, 2017 08:57

youknowriad merged commit 29462aa into master May 2, 2017

youknowriad deleted the add/link-control-inline branch May 2, 2017 14:32

aduth reviewed May 2, 2017

View reviewed changes

snyk-bot mentioned this pull request Jul 28, 2021

[Snyk] Upgrade uuid from 8.3.0 to 8.3.2 MaxMood96/gutenberg#3

Open

snyk-bot mentioned this pull request Jan 15, 2023

[Snyk] Upgrade uuid from 8.3.0 to 8.3.2 yassine4999/gutenberg#2

Open

Editable: Adding an inline link control to the format toolbar #524

Editable: Adding an inline link control to the format toolbar #524

Conversation

youknowriad commented Apr 27, 2017

youknowriad commented Apr 27, 2017

jasmussen commented Apr 27, 2017

aduth commented Apr 27, 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

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

ellatrix commented Apr 28, 2017

ellatrix commented Apr 28, 2017

ellatrix commented Apr 28, 2017 • edited Loading

youknowriad commented Apr 28, 2017

aduth commented Apr 28, 2017

youknowriad commented Apr 28, 2017

aduth commented Apr 28, 2017

youknowriad commented May 1, 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

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 commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Apr 28, 2017 •

edited

Loading