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

Always

Just for now

add/2793: Improved inline documentation

  • Loading branch information...
EphoxJames committed Oct 10, 2017
commit 2386c1fc2b593d7524f7388a74c9139bdbf97b13
@@ -5,10 +5,62 @@ 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 {String} key unique string to help react render efficiently.
* @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 {
key: name,
value: name,
label: [
<BlockIcon key="icon" icon={ icon } />,
@@ -24,9 +76,7 @@ export function blockAutocompleter( { onReplace } ) {
return ! ( /\S/.test( before.toString() ) || /\S/.test( after.toString() ) );
};

const onSelect = ( option ) => {
const { value: blockName } = option;

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

@@ -38,13 +88,19 @@ export function blockAutocompleter( { onReplace } ) {
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 ) => {

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

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.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

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

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@afercia

afercia Oct 20, 2017

Contributor

@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>,
@@ -60,8 +116,7 @@ export function userAutocompleter() {
return textNode.parentElement.closest( 'a' ) === null;
};

const onSelect = ( option, range ) => {
const { user } = option;
const onSelect = ( user, range ) => {
const mention = document.createElement( 'a' );
mention.href = user.link;
mention.textContent = '@' + user.name;
@@ -20,6 +20,11 @@ import Popover from '../popover';

const { ENTER, ESCAPE, UP, DOWN } = keycodes;

/**
* Recursively select the firstChild until hitting a leaf node.
* @param {Node} node the node to find the recursive first child.
* @returns {Node} the first leaf-node >= node in the ordering.
*/
function descendFirst( node ) {
let n = node;
while ( n.firstChild ) {
@@ -28,6 +33,11 @@ function descendFirst( node ) {
return n;
}

/**
* Recursively select the lastChild until hitting a leaf node.
* @param {Node} node the node to find the recursive last child.
* @returns {Node} the first leaf-node <= node in the ordering.
*/
function descendLast( node ) {
let n = node;
while ( n.lastChild ) {
@@ -36,14 +46,30 @@ function descendLast( node ) {
return n;
}

/**
* Is the node a text node.
* @param {?Node} node the node to check.
* @returns {boolean} true if the node is a text node.
*/
function isTextNode( node ) {
return node !== null && node.nodeType === 3;
}

/**
* Return the node only if it is a text node, otherwise return null.
* @param {?Node} node the node to filter.
* @returns {?Node} the node or null if it is not a text node.
*/
function onlyTextNode( node ) {
return isTextNode( node ) ? node : null;
}

/**
* Find the index of the last charater in the text that is whitespace.
* @param {String} text the text to search.
* @param {Number} fromIndex the index to start from.
* @returns {Number} the last index of a white space character in the text or -1.
*/
function lastIndexOfSpace( text, fromIndex ) {
const fromI = fromIndex === undefined ?
text.length - 1 :
@@ -91,7 +117,7 @@ class Autocomplete extends Component {
this.reset();

if ( onSelect ) {
onSelect( option, range, lookup );
onSelect( option.value, range, lookup );
}
}

@@ -300,6 +326,11 @@ class Autocomplete extends Component {
}

toggleKeyEvents( isListening ) {
// This exists because we must capture ENTER key presses before Editable.

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

If enter works correctly in master with the block autocomplete, why do we suddenly need this now?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

The enter key handler in master relied on overriding the keyhandler of the Editable. I'm not certain why it was able to get the event before TinyMCE when a onKeyDownCapture handler on the surrounding component was not - I guess the react simulated events don't mix with native events perfectly. When I was initially working on nesting these autocomplete handlers that approach was unworkable though now it is only one component I suppose we could return to it. That said I think that overriding the event handler on a child block is very weird and I much prefer this approach.

// It seems that react fires the simulated capturing events after the
// native browser event has already bubbled so we can't stopPropagation
// and avoid Editable getting the event from TinyMCE, hence we must
// register a native event handler.
if ( ! this.node ) {

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

When is this.node falsey?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

This won't be called very often so I doubt it will have much effect on performance but I have removed the guard as requested.

This comment has been minimized.

Copy link
@aduth

aduth Nov 3, 2017

Member

It's less about performance or safe-guarding, more to having knowledge to when those safe-guards should be expected to be encountered.

return;
}
@@ -310,9 +341,11 @@ class Autocomplete extends Component {

componentWillMount() {
const { completers } = this.props;
// Some completers must do asynchronous requests to get their options so
// we calculate that here.
completers.forEach( ( { getOptions }, idx ) => {
getOptions().then( ( options ) => {

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

This behavior means we're issuing one request to the users endpoint for every paragraph block in a post. I'm not sure we need to be so proactive about querying this either? In some cases won't we need fresh data in response to a user search?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

Good point - I will change it to populate the cache on the first non-empty search of a lookup.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

Now it loads options on first use.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

I have tweaked it so it also makes the getOptions call each time the autocompleter is initially triggered, so the information will be fresh, though query characters that narrow down the search will not cause a new getOptions call.

this.allOptions[ idx ] = options;
this.allOptions[ idx ] = options.map( ( option, key ) => ( { ...option, key } ) );
this.forceUpdate();

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

Rather than forcing the update, should we be setting all options into state via setState?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

Yeah I probably should. I did it this way because I was too lazy to rewrite the reset function (which would have deleted the cache the way it is currently written).

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

I have moved the options into state.

} );
} );
@@ -337,8 +370,6 @@ class Autocomplete extends Component {
const classes = classnames( 'components-autocomplete__popover', className );
const filteredOptions = this.getFilteredOptions();

// Blur is applied to the wrapper node, since if the child is Editable,
// the event will not have `relatedTarget` assigned.
return (
<div
ref={ this.bindNode }
@@ -360,7 +391,7 @@ class Autocomplete extends Component {
>
{ filteredOptions.map( ( option, index ) => (
<li
key={ option.value }
key={ option.key.toString() }

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

What's the case where the toString becomes necessary?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

According to react docs key should be a String - I wanted to use a number so I converted it to a string.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

From this page:
https://reactjs.org/docs/lists-and-keys.html
"A “key” is a special string attribute you need to include when creating lists of elements. "

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

Anyway I have now changed this so the key is the combined indexes of the completer and the option separated by underscore so the toString is getting removed.

role="menuitem"
className={ classnames( 'components-autocomplete__result', {
'is-selected': index === selectedIndex,
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.