Editor: Add components for editor mentions #10142

donnapep · 2016-12-17T21:57:56Z

To facilitate merging, this PR separates out the React components from PR #8028, which adds support for mentions to the editor. Still to come is a PR that leverages these components.

Dependent on PR #9969.

cc @mtias

bluefuton · 2017-01-12T06:50:45Z

client/components/tinymce/plugins/mentions/mentions.jsx

+/**
+ * Module variables
+ */
+const VK = tinymce.util.VK;


May be worth us adding a comment to explain what TinyMCE's rather cryptically-named VK is for.

Looks like this is a partial mapping of keycodes: https://github.com/tinymce/tinymce/blob/master/js/tinymce/classes/util/VK.js

gwwar · 2017-01-12T23:28:39Z

client/components/tinymce/plugins/mentions/mentions.jsx

+	return suggestions
+		.filter( ( { user_login, display_name } ) => {
+			// Start of string or preceded by a space.
+			const matcher = new RegExp( `^${ query }|\\s${ query }`, 'ig' );


Do usernames ever use reserved regex characters? Any reason for testing against a single whitespace vs many?

An alternative might just be to use lodash trim and startsWith

Re: whitespace. It's matching against ${ user_login } ${ display_name }, so whether the regex uses \s or \s+, the result would be the same.

I also tried creating a user with a user name starting with ^ and it wasn't allowed.

gwwar · 2017-01-12T23:31:30Z

client/components/tinymce/plugins/mentions/mentions.jsx

+
+	constructor( props ) {
+		super( props );
+	}


Doesn't look like we need a custom constructor here.

gwwar · 2017-01-12T23:55:16Z

client/components/tinymce/plugins/mentions/mentions.jsx

+
+	updatePosition( state, { left, top } = this.getPosition( state ) ) {
+		const { editor, node } = this.props;
+		const mceToolbar = tinymce.$( '.mce-toolbar-grp', editor.getContainer() )[ 0 ];


It's unlikely to happen, but should we bulletproof this if we can't find the node?

gwwar · 2017-01-13T00:03:50Z

client/components/tinymce/plugins/mentions/mentions.jsx

+		return null;
+	}
+
+	getSelectedSuggestionIndex() {


I think we can use findIndex here. If you pass through undefined or null as the first argument, this method will return -1.

if ( ! this.state.selectedSuggestionId ) { return 0; } return findIndex( this.matchingSuggestions, ( suggestion ) => { return suggestion.ID === this.state.selectedSuggestionId; } );

I'd also suggest updating the no-match value from null to -1, which is common for these indexOf functions.

gwwar · 2017-01-13T00:07:25Z

client/components/tinymce/plugins/mentions/mentions.jsx

+
+		for ( let i = 0; i < this.matchingSuggestions.length; i++ ) {
+			if ( this.matchingSuggestions[ i ].ID === this.state.selectedSuggestionId ) {
+				return this.matchingSuggestions[ i ];


How about:

const index = this.getSelectedSuggestionIndex(); if ( index >= 0 ) { return this.matchingSuggestions[ index ]; } return null;

gwwar · 2017-01-13T00:08:23Z

client/components/tinymce/plugins/mentions/mentions.jsx

+
+	insertSuggestion = ( suggestion ) => {
+		const { editor } = this.props;
+		const re = /@\S*/;


If this never changes, we can move this up out of the function as a constant;

gwwar · 2017-01-13T00:11:44Z

client/components/tinymce/plugins/mentions/mentions.jsx

+	insertSuggestion = ( suggestion ) => {
+		const { editor } = this.props;
+		const re = /@\S*/;
+		const markup = <EditorMention username={ suggestion.user_login } />;


Should we do an early return if suggestion is falsey?

gwwar · 2017-01-13T00:14:03Z

client/components/tinymce/plugins/mentions/mentions.jsx

+				return;
+			}
+
+			if ( keyCode === VK.DOWN ) {


Maybe move the prevent default to above this line?

Also a switch might read a little nicer here, though that's optional.

See #10142 (review) See #10142 (review)

See #10142 (comment)

obenland · 2017-02-25T03:46:57Z

Let's give this another round of eyes? @gwwar most of your concerns have been addressed I think, with the exception of your regex question. Maybe @donnapep has more insights there?

gwwar · 2017-02-28T01:35:35Z

client/components/tinymce/plugins/mentions/mentions.jsx

+		if ( query ) {
+			suggestions = suggestions.filter( ( { user_login: userLogin, display_name: displayName } ) => {
+				// Start of string or preceded by a space.
+				const matcher = new RegExp( `^${ query }|\\s${ query }`, 'ig' );


I just want to verify that we intend to enable regex searches here. For example a user typing "foo.bar" for query will match the literal "foo.bar" along with any strings like "fooxbar" where the period can be replaced with any character.

If we intend to only do literal searches, we'd want to escape any reserved regex characters from the query.

Ah, I see what you're saying. Perhaps we can address this in #10321

gwwar

Unless folks have any objections, let's 🚢

Even though this change should be pretty safe. Let's also remember to do a bit of smoke testing on stage before deploying

obenland · 2017-02-28T02:40:25Z

w00t! 🎉

Just so I understand correctly, when you say smoke-testing, do you mean with #10321?

mtias · 2017-02-28T11:23:23Z

Nice to see this being merged!

gwwar · 2017-02-28T12:00:24Z

Just so I understand correctly, when you say smoke-testing, do you mean with #10321?

@obenland No I meant this PR. Safe changes have led to folks have skipping this step in the past, which sometimes causes issues.

donnapep · 2017-02-28T12:31:06Z

@gwwar I poked around the editor a bit before deploying and didn't see any issues. Thanks for the review!

gwwar · 2017-02-28T23:20:21Z

client/components/tinymce/plugins/mentions/mentions.jsx

+			} );
+		}
+
+		return suggestions.slice( 0, 10 );


We can use lodash slice here for safety, or add a falsey check. soslice( suggestions, 0, 10 ) or suggestions && suggestions.slice( 0, 10 );

Also do we ever expect to have suggestions if query is falsey?

I was just looking at getUserSuggestions() and I wonder if it would make sense to fall back to an empty array there?

Yeah that looks like a better default.

matticbot added the OSS Citizen label Dec 17, 2016

donnapep added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 17, 2016

donnapep mentioned this pull request Dec 29, 2016

Editor: Make TinyMCE editor mentions plugin available behind a feature flag #10321

Merged

donnapep force-pushed the add/editor-mentions-components branch from 896d623 to b6a176e Compare December 29, 2016 17:39

gwwar self-requested a review January 11, 2017 22:59

bluefuton mentioned this pull request Jan 12, 2017

Editor: Add query component for fetching user suggestions #9969

Merged

bluefuton reviewed Jan 12, 2017

View reviewed changes

gwwar reviewed Jan 12, 2017

View reviewed changes

gwwar reviewed Jan 13, 2017

View reviewed changes

mtias added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 24, 2017

donnapep mentioned this pull request Feb 7, 2017

Editor: Add support for @mentions #8028

Closed

donnapep force-pushed the add/editor-mentions-components branch from ab28552 to 88c3e6e Compare February 24, 2017 20:27

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 24, 2017

obenland force-pushed the add/editor-mentions-components branch from 2b059c0 to 1ee6862 Compare February 25, 2017 00:05

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 25, 2017

obenland force-pushed the add/editor-mentions-components branch from 1ee6862 to c775c8f Compare February 25, 2017 00:12

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 25, 2017

Add EditorMention component

458a987

Donna Peplinskie and others added 8 commits February 24, 2017 19:32

Add Suggestion component

a8c1528

Add SuggestionList component

65e6e9a

Add Mentions component

b8c1d03

Remove unnecessary constructor from Mentions component

43a1048

Execute preventDefault sooner

fe129cb

Return early from insertSuggestion if no suggestion

cb3d658

Simplify suggestions indices and retrieval

752eafc

See #10142 (review) See #10142 (review)

Add a fallback in case the toolbar can't be found

f0d0f89

See #10142 (comment)

obenland force-pushed the add/editor-mentions-components branch from c775c8f to f0d0f89 Compare February 25, 2017 03:32

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 25, 2017

obenland added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Feb 25, 2017

gwwar reviewed Feb 28, 2017

View reviewed changes

gwwar approved these changes Feb 28, 2017

View reviewed changes

donnapep merged commit e8e4bb0 into master Feb 28, 2017

donnapep deleted the add/editor-mentions-components branch February 28, 2017 11:13

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 28, 2017

gwwar reviewed Feb 28, 2017

View reviewed changes

bluefuton mentioned this pull request Apr 16, 2018

Add UserMentionSuggestions block #24194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Add components for editor mentions #10142

Editor: Add components for editor mentions #10142

donnapep commented Dec 17, 2016

bluefuton Jan 12, 2017

gwwar Jan 12, 2017

gwwar Jan 12, 2017

donnapep Feb 27, 2017

gwwar Jan 12, 2017

gwwar Jan 12, 2017

gwwar Jan 13, 2017 •

edited

Loading

gwwar Jan 13, 2017 •

edited

Loading

gwwar Jan 13, 2017

gwwar Jan 13, 2017

gwwar Jan 13, 2017

obenland commented Feb 25, 2017

gwwar Feb 28, 2017

donnapep Feb 28, 2017

gwwar left a comment

obenland commented Feb 28, 2017

mtias commented Feb 28, 2017

gwwar commented Feb 28, 2017

donnapep commented Feb 28, 2017

gwwar Feb 28, 2017

obenland Feb 28, 2017

gwwar Mar 1, 2017

Editor: Add components for editor mentions #10142

Editor: Add components for editor mentions #10142

Conversation

donnapep commented Dec 17, 2016

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

gwwar Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

gwwar Jan 13, 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

obenland commented Feb 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar left a comment

Choose a reason for hiding this comment

obenland commented Feb 28, 2017

mtias commented Feb 28, 2017

gwwar commented Feb 28, 2017

donnapep commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar Jan 13, 2017 •

edited

Loading

gwwar Jan 13, 2017 •

edited

Loading