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.

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

Autocomplete links to user posts #2896

Merged
merged 50 commits into from Nov 3, 2017
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
2667d2d
Autocomplete links to user posts
tiny-james Oct 6, 2017
96ed19e
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 9, 2017
5db79cd
add/2793: Changed autocomplete to take in "completer" definitions
tiny-james Oct 9, 2017
fe4aa22
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 10, 2017
2386c1f
add/2793: Improved inline documentation
tiny-james Oct 10, 2017
d8d91b4
add/2793: Removed old autocomplete components
tiny-james Oct 11, 2017
053a478
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 11, 2017
fc53c3b
add/2793: Updating tests [WIP]
tiny-james Oct 11, 2017
46875ad
add/2793: Updated the rest of the existing tests
tiny-james Oct 13, 2017
ad8d036
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 13, 2017
f5eb752
add/2793: Removed blank line that linter complained about
tiny-james Oct 15, 2017
ad93a3e
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 15, 2017
29b0a36
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 17, 2017
a23a608
add/2793: Allow onSelect to optionally return selection replacement
tiny-james Oct 17, 2017
e2988b4
add/2793: Made linter happy again
tiny-james Oct 17, 2017
4c0c793
add/2793: extract out findMatch; load options on demand; handle tab
tiny-james Oct 18, 2017
ca6c469
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 18, 2017
83d697e
add/2793: Hide the popover on left and right arrow keys
tiny-james Oct 18, 2017
22baa78
add/2793: Moved options into state and reload them on each new open
tiny-james Oct 18, 2017
3c5350a
add/2793: Removed unused allOptions variable
tiny-james Oct 18, 2017
94110cd
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 18, 2017
5220299
add/2793: changed onClickOutside to onClose
tiny-james Oct 19, 2017
158c9d0
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 20, 2017
8791dd3
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 23, 2017
79712b8
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 24, 2017
56c0889
add/2793: Updated tests to work with updated enzyme
tiny-james Oct 24, 2017
58f703e
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 25, 2017
ed9bd85
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 30, 2017
f48d7e1
Fixed linting error.
tiny-james Oct 30, 2017
8ae2e80
add/2793: Try to make autocomplete popover accessable
tiny-james Oct 30, 2017
cb5aa58
add/2793: Updated autocomplete tests to handle FACC structure
tiny-james Oct 30, 2017
f6234ae
add/2793: delegate handling of nil aria attributes to Editable
tiny-james Oct 30, 2017
7089532
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Oct 31, 2017
4f437e3
add/2793: Disabled linter warning about findDOMNode as it is required
tiny-james Oct 31, 2017
2207026
add/2793: Converted autocomplete FACC parameters into object
tiny-james Nov 2, 2017
2c75ae1
add/2793: Moved test that selection is within container into assertion
tiny-james Nov 2, 2017
aa420d2
add/2793: Converted `Array#map` and `Array#filter` to lodash equalivents
tiny-james Nov 2, 2017
41e10da
add/2793: Reorder calculations to avoid doing unneeded work
tiny-james Nov 2, 2017
5a25e32
add/2793: Break out of loop early to avoid excess indentation
tiny-james Nov 2, 2017
c06e95e
add/2793: Removed unused 'range' parameter of allowContext
tiny-james Nov 2, 2017
dccc61e
add/2793: Removed unneeded if-statment
tiny-james Nov 2, 2017
3d7ffd5
add/2793: Avoid some indentation with an early return
tiny-james Nov 2, 2017
bdae604
add/2793: Replace escapeStringRegexp with escapeRegExp from lodash
tiny-james Nov 2, 2017
a25f6f7
add/2793: Removed unneeded guard statement
tiny-james Nov 2, 2017
78221f5
add/2793: Avoid spurious react reconcile by unwrapping function
tiny-james Nov 2, 2017
4e995dc
add/2793: Moved bounding client rect calculation into else branch
tiny-james Nov 2, 2017
20fa7fb
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Nov 2, 2017
bbae623
add/2793: Only take padding into account when using the anchor
tiny-james Nov 2, 2017
b9871c6
Add/2793: Allow popover position to be overriden by prop getAnchorRect
tiny-james Nov 3, 2017
8fc7023
Merge branch 'master' into add/2793-user-autocompletion
tiny-james Nov 3, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
132 changes: 132 additions & 0 deletions blocks/autocompleters/index.js
@@ -0,0 +1,132 @@
/**
* Internal dependencies
*/
import './style.scss';
import { createBlock, getBlockTypes } from '../api';
import BlockIcon from '../block-icon';

/**
* @typedef {Object} CompleterOption
* @property {Array.<Component>} label list of react components to render.
* @property {Array.<String>} keywords list of key words to search.
* @property {*} value the value that will be passed to onSelect.
*/

/**
* @callback FnGetOptions
* @returns {Promise.<Array.<CompleterOption>>} A promise that resolves to the list of completer options.
*/

/**
* @callback FnAllowNode
* @param {Node} textNode check if the completer can handle this text node.
* @returns {boolean} true if the completer can handle this text node.
*/

/**
* @callback FnAllowContext
* @param {Range} before the range before the auto complete trigger and query.
* @param {Range} range the range of the autocomplete trigger and query.
* @param {Range} after the range after the autocomplete trigger and query.
* @returns {boolean} true if the completer can handle these ranges.
*/

/**
* @callback FnOnSelect
* @param {*} value the value of the completer option.
* @param {Range} range the nodes included in the autocomplete trigger and query.
* @param {String} query the text value of the autocomplete query.
*/

/**
* @typedef {Object} Completer
* @property {?String} className A class to apply to the popup menu.
* @property {String} triggerPrefix the prefix that will display the menu.
* @property {FnGetOptions} getOptions get the block options in a resolved promise.
* @property {?FnAllowNode} allowNode filter the allowed text nodes in the autocomplete.
* @property {?FnAllowContext} allowContext filter the context under which the autocomplete activates.
* @property {FnOnSelect} onSelect
*/

/**
* Returns an "completer" definition for selecting from available blocks to replace the current one.
* The definition can be understood by the Autocomplete component.
*
* @param {Function} onReplace Callback to replace the current block.
* @returns {Completer} Completer object used by the Autocomplete component.
*/
export function blockAutocompleter( { onReplace } ) {
const options = getBlockTypes().map( ( blockType ) => {
const { name, title, icon, keywords = [] } = blockType;
return {
value: name,
label: [
<BlockIcon key="icon" icon={ icon } />,
title,
],
keywords: [ ...keywords, title ],
};
} );

const getOptions = () => Promise.resolve( options );

const allowContext = ( before, range, after ) => {
return ! ( /\S/.test( before.toString() ) || /\S/.test( after.toString() ) );
};

const onSelect = ( blockName ) => {
onReplace( createBlock( blockName ) );
};

return {
className: 'blocks-block-autocomplete',
triggerPrefix: '/',
getOptions,
allowContext,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't understand the difference between triggerPrefix and allowContext, could these be merged into one function?

Copy link
Author

Choose a reason for hiding this comment

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

TLDR: not without making it an annoying API to use.

triggerPrefix is the string that preceeds the query. For example "@" in the case of the user mentions, "/" in the case of block selection and "#" in the case of hash-tags (which aren't implemented yet). In theory you could just search back to the last space or boundary and pass the whole range to the completer definition to check but that that means that ALL completers would have to convert the range into text and check the leading substring. This puts more work on the implementer of the completer which is exactly where we don't want it.

allowContext was added to solve the problem that sometimes the completer has to know what preceeds it and follows it. The majority of the time this doesn't matter but the original implementation of the autocomplete block would require that there was nothing but whitespace before and afterwards (which makes sense because it replaces the whole block). This field is completely optional and most completers won't use it.

onSelect,
};
}
/**
* Returns a "completer" definition for inserting links to the posts of a user.
* The definition can be understood by the Autocomplete component.
*
* @returns {Completer} Completer object used by the Autocomplete component.
*/
export function userAutocompleter() {
const getOptions = () => {
return ( new wp.api.collections.Users() ).fetch().then( ( users ) => {
Copy link
Member

Choose a reason for hiding this comment

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

We might consider the implications for users who don't have permissions to list other users on a site. Currently this doesn't appear to break anything, but will only ever return administrator users if issued by a non-administrator. Probably fine, with the other option being that we don't enable the autocompleter for these accounts.

Copy link
Author

Choose a reason for hiding this comment

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

If you know of a better source for the information I will happily switch to it.

return users.map( ( user ) => {
return {
value: user,
label: [
<img key="avatar" alt="" src={ user.avatar_urls[ 24 ] } />,
Copy link
Member

Choose a reason for hiding this comment

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

We should assign a meaningful alt here, maybe { __( 'User avatar' ) }

Copy link
Author

Choose a reason for hiding this comment

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

The problem with that is that screen readers will (for each item) read out "User avatar" before telling them the actually useful information of the user-name. Unless we can give an actually useful description it's probably more usable if they are just treated as decoration.

Copy link
Author

Choose a reason for hiding this comment

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

@afercia do you think we should put descriptive text on the avatar in the users menu or treat it as decorative?

Copy link
Contributor

Choose a reason for hiding this comment

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

@EphoxJames empty alt are fine in this case. The relevant information is already in plain text close to the image, so these images are decorative.
Although not exactly the same thing, we've faced a similar case in the WP credits page. See https://core.trac.wordpress.org/changeset/36406 and https://core.trac.wordpress.org/ticket/34953.

<span key="name" className="name">{ user.name }</span>,
<span key="slug" className="slug">{ user.slug }</span>,
],
keywords: [ user.slug, user.name ],
};
} );
} );
};

const allowNode = ( textNode ) => {
return textNode.parentElement.closest( 'a' ) === null;
};

const onSelect = ( user, range ) => {
const mention = document.createElement( 'a' );
mention.href = user.link;
mention.textContent = '@' + user.name;
range.insertNode( mention );
range.setStartAfter( mention );
range.deleteContents();
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the userAutocompleter should be aware of what to do with the selected value. This forces us to add a range property here where we could just execute a TinyMCE command if we were to have an instance of it.

Same could be said above for the blocks autocompleter, assuming there's only one action to be done when selecting a block: replacing.

What if the onSelect is always provided by the calling side:

<Autocomplete key="editable" completers={ [
	blockAutocompleter( block => onReplace( block ) ),
	userAutocompleter( user => this.editor.insertContent( linkFromUser( user ) ) ),
] }>

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the difficulty here is that we need to remove what we already typed. Maybe we should do this consistently before calling the onSelect

Copy link
Author

Choose a reason for hiding this comment

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

The advantage of Range is that it is an editor agnostic API and in the past I have been told I can't assume that components will always work with TinyMCE. I specifically did not want the autocomplete code to make any assumptions about what would be done with the range.
Note that TinyMCE doesn't really care about us changing the content out from under it and it would be breaking the abstraction that Editable is suppose to provide.

Copy link
Author

Choose a reason for hiding this comment

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

Also one of the advantages of using range is that TinyMCE knows how to work with ranges.
For example if TinyMCE was available as this.editor then you could do:

(user, range) => {
  this.editor.selection.setRng( range );
  this.editor.insertContent( linkFromUser( user ) );
}

That would select the already typed content and immediately replace it with the inserted content.

};

return {
className: 'blocks-user-autocomplete',
triggerPrefix: '@',
getOptions,
allowNode,
onSelect,
};
}
12 changes: 12 additions & 0 deletions blocks/autocompleters/style.scss
@@ -0,0 +1,12 @@
.blocks-block-autocomplete .dashicon {
margin-right: 8px;
}

.blocks-user-autocomplete img {
margin-right: 8px;
}

.blocks-user-autocomplete .slug {
margin-left: 8px;
color: $dark-gray-100;
}
56 changes: 0 additions & 56 deletions blocks/block-autocomplete/index.js

This file was deleted.

3 changes: 0 additions & 3 deletions blocks/block-autocomplete/style.scss

This file was deleted.

60 changes: 0 additions & 60 deletions blocks/block-autocomplete/test/index.js

This file was deleted.

13 changes: 8 additions & 5 deletions blocks/library/paragraph/index.js
Expand Up @@ -8,17 +8,17 @@ import classnames from 'classnames';
*/
import { __ } from '@wordpress/i18n';
import { concatChildren } from '@wordpress/element';
import { PanelBody } from '@wordpress/components';
import { Autocomplete, PanelBody } from '@wordpress/components';

/**
* Internal dependencies
*/
import './style.scss';
import { registerBlockType, createBlock, source, setDefaultBlockName } from '../../api';
import { blockAutocompleter, userAutocompleter } from '../../autocompleters';
import AlignmentToolbar from '../../alignment-toolbar';
import BlockAlignmentToolbar from '../../block-alignment-toolbar';
import BlockControls from '../../block-controls';
import BlockAutocomplete from '../../block-autocomplete';
import Editable from '../../editable';
import InspectorControls from '../../inspector-controls';
import ToggleControl from '../../inspector-controls/toggle-control';
Expand Down Expand Up @@ -152,7 +152,10 @@ registerBlockType( 'core/paragraph', {
</PanelBody>
</InspectorControls>
),
<BlockAutocomplete key="editable" onReplace={ onReplace }>
<Autocomplete key="editable" completers={ [
blockAutocompleter( { onReplace } ),
userAutocompleter(),
] }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking the "patterns" we have right now in Editable could be written like this cc @iseulde

<Editable
tagName="p"
className={ classnames( 'wp-block-paragraph', className, {
Expand Down Expand Up @@ -183,8 +186,8 @@ registerBlockType( 'core/paragraph', {
onMerge={ mergeBlocks }
onReplace={ onReplace }
placeholder={ placeholder || __( 'New Paragraph' ) }
/>
</BlockAutocomplete>,
/>
</Autocomplete>,
];
},

Expand Down