update/3329: Changed autocomplete menu "ul > li > button" to "div > button" #3343
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { escapeRegExp, find, filter, map } from 'lodash'; | |
| */ | ||
| import { Component, renderToString } from '@wordpress/element'; | ||
| import { keycodes } from '@wordpress/utils'; | ||
| import { __, _n, sprintf } from '@wordpress/i18n'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
|
|
@@ -17,8 +18,9 @@ import './style.scss'; | |
| import Button from '../button'; | ||
| import Popover from '../popover'; | ||
| import withInstanceId from '../higher-order/with-instance-id'; | ||
| import withSpokenMessages from '../higher-order/with-spoken-messages'; | ||
|
|
||
| const { ENTER, ESCAPE, UP, DOWN, LEFT, RIGHT, TAB, SPACE } = keycodes; | ||
|
|
||
| /** | ||
| * Recursively select the firstChild until hitting a leaf node. | ||
|
|
@@ -78,14 +80,43 @@ function lastIndexOfSpace( text ) { | |
| return -1; | ||
| } | ||
|
|
||
| function filterOptions( search, options = [], maxResults = 10 ) { | ||
| const filtered = []; | ||
| for ( let i = 0; i < options.length; i++ ) { | ||
| const option = options[ i ]; | ||
|
|
||
| // Merge label into keywords | ||
| let { keywords = [] } = option; | ||
| if ( 'string' === typeof option.label ) { | ||
| keywords = [ ...keywords, option.label ]; | ||
| } | ||
|
|
||
| const isMatch = keywords.some( ( keyword ) => search.test( keyword ) ); | ||
| if ( ! isMatch ) { | ||
| continue; | ||
| } | ||
|
|
||
| filtered.push( option ); | ||
|
|
||
| // Abort early if max reached | ||
| if ( filtered.length === maxResults ) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return filtered; | ||
| } | ||
|
|
||
| export class Autocomplete extends Component { | ||
| static getInitialState() { | ||
| return { | ||
| search: /./, | ||
| selectedIndex: 0, | ||
| suppress: undefined, | ||
| open: undefined, | ||
| query: undefined, | ||
| range: undefined, | ||
| filteredOptions: [], | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -161,12 +192,33 @@ export class Autocomplete extends Component { | |
| return range; | ||
| } | ||
|
|
||
| announce( filteredOptions ) { | ||
| const { debouncedSpeak } = this.props; | ||
| if ( ! debouncedSpeak ) { | ||
| return; | ||
| } | ||
| if ( !! filteredOptions.length ) { | ||
| debouncedSpeak( sprintf( _n( | ||
| '%d result found, use up and down arrow keys to navigate.', | ||
| '%d results found, use up and down arrow keys to navigate.', | ||
| filteredOptions.length | ||
| ), filteredOptions.length ), 'assertive' ); | ||
| } else { | ||
| debouncedSpeak( __( 'No results.' ), 'assertive' ); | ||
| } | ||
| } | ||
|
|
||
| loadOptions( index ) { | ||
| this.props.completers[ index ].getOptions().then( ( options ) => { | ||
| const keyedOptions = map( options, ( option, i ) => ( { ...option, key: index + '-' + i } ) ); | ||
| const filteredOptions = filterOptions( this.state.search, keyedOptions ); | ||
| const selectedIndex = filteredOptions.length === this.state.filteredOptions.length ? this.state.selectedIndex : 0; | ||
| this.setState( { | ||
| [ 'options_' + index ]: keyedOptions, | ||
| filteredOptions, | ||
| selectedIndex, | ||
| } ); | ||
| this.announce( filteredOptions ); | ||
| } ); | ||
| } | ||
|
|
||
|
|
@@ -247,88 +299,67 @@ export class Autocomplete extends Component { | |
| } | ||
|
|
||
| search( event ) { | ||
| const { open: wasOpen, suppress: wasSuppress } = this.state; | ||
| const { completers } = this.props; | ||
| // ensure that the cursor location is unambiguous | ||
| const container = event.target; | ||
| const cursor = this.getCursor( container ); | ||
| // look for the trigger prefix and search query just before the cursor location | ||
| const match = this.findMatch( container, cursor, completers, wasOpen ); | ||
| const { open, query, range } = match || {}; | ||
| // asynchronously load the options for the open completer | ||
| if ( open && ( ! wasOpen || open.idx !== wasOpen.idx ) ) { | ||
| this.loadOptions( open.idx ); | ||
| } | ||
| // create a regular expression to filter the options | ||
| const search = open ? new RegExp( '(?:\\b|\\s|^)' + escapeRegExp( query ), 'i' ) : /./; | ||
| // filter the options we already have | ||
| const filteredOptions = open ? filterOptions( search, this.state[ 'options_' + open.idx ] ) : []; | ||
| // check if we should still suppress the popover | ||
| const suppress = open && wasSuppress === open.idx ? wasSuppress : undefined; | ||
|
There was a problem hiding this comment. I suggest adding parentheses to make eval order really obvious, instead of relying on precedence rules. One level is enough: |
||
| // update the state | ||
| if ( wasOpen || open ) { | ||
| this.setState( { selectedIndex: 0, filteredOptions, suppress, search, open, query, range } ); | ||
| } | ||
| // announce the count of filtered options but only if they have loaded | ||
| if ( open && this.state[ 'options_' + open.idx ] ) { | ||
| this.announce( filteredOptions ); | ||
| } | ||
| } | ||
|
|
||
| setSelectedIndex( event ) { | ||
| const { open, suppress, selectedIndex, filteredOptions } = this.state; | ||
| if ( ! open || filteredOptions.length === 0 ) { | ||
| return; | ||
| } | ||
| if ( suppress === open.idx ) { | ||
| const { keyCode, ctrlKey, shiftKey, altKey, metaKey } = event; | ||
| if ( keyCode === SPACE && ctrlKey && ! ( shiftKey || altKey || metaKey ) ) { | ||
|
There was a problem hiding this comment. I'm wondering about the
There was a problem hiding this comment. In a future refactor, to simplify the handling of modifier keys, we might consider adding support to There was a problem hiding this comment. Yep, that still hidden after move thing is a bug. |
||
| this.setState( { suppress: undefined } ); | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| let nextSelectedIndex; | ||
| switch ( event.keyCode ) { | ||
| case UP: | ||
| nextSelectedIndex = ( selectedIndex === 0 ? filteredOptions.length : selectedIndex ) - 1; | ||
| this.setState( { selectedIndex: nextSelectedIndex } ); | ||
| break; | ||
|
|
||
| case DOWN: | ||
| nextSelectedIndex = ( selectedIndex + 1 ) % filteredOptions.length; | ||
| this.setState( { selectedIndex: nextSelectedIndex } ); | ||
| break; | ||
|
|
||
| case ESCAPE: | ||
| this.setState( { suppress: open.idx } ); | ||
| break; | ||
|
|
||
| case ENTER: | ||
| this.select( filteredOptions[ selectedIndex ] ); | ||
| break; | ||
|
|
||
| case LEFT: | ||
|
|
@@ -386,13 +417,13 @@ export class Autocomplete extends Component { | |
|
|
||
| render() { | ||
| const { children, instanceId } = this.props; | ||
| const { open, suppress, selectedIndex, filteredOptions } = this.state; | ||
| const { key: selectedKey = '' } = filteredOptions[ selectedIndex ] || {}; | ||
| const { className, idx } = open || {}; | ||
| const classes = classnames( 'components-autocomplete__popover', className ); | ||
| const isExpanded = suppress !== idx && filteredOptions.length > 0; | ||
| const listBoxId = isExpanded ? `components-autocomplete-listbox-${ instanceId }` : null; | ||
| const activeId = isExpanded ? `components-autocomplete-item-${ instanceId }-${ selectedKey }` : null; | ||
|
|
||
| return ( | ||
| <div | ||
|
|
@@ -409,31 +440,30 @@ export class Autocomplete extends Component { | |
| className={ classes } | ||
| getAnchorRect={ this.getWordRect } | ||
| > | ||
| <div | ||
| id={ listBoxId } | ||
| role="listbox" | ||
| className="components-autocomplete__results" | ||
| > | ||
| { isExpanded && map( filteredOptions, ( option, index ) => ( | ||
| <Button | ||
| key={ option.key } | ||
| id={ `components-autocomplete-item-${ instanceId }-${ option.key }` } | ||
| role="option" | ||
| aria-selected={ index === selectedIndex } | ||
| className={ classnames( 'components-autocomplete__result', { | ||
| 'is-selected': index === selectedIndex, | ||
| } ) } | ||
| onClick={ () => this.select( option ) } | ||
| > | ||
| { option.label } | ||
| </Button> | ||
| ) ) } | ||
| </div> | ||
| </Popover> | ||
| </div> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| export default withSpokenMessages( withInstanceId( Autocomplete ) ); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although they weren't introduced in this PR, these classnames don't adhere to Gutenberg's BEM-like conventions. We should have something along the lines of
components-autocomplete__result-name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, reading that document I'd come up with
blocks-autocompleters__user-namebecause the actual CSS is not in thecomponents/autocompletefolder but theblocks/autocompletersfolder. @mcsf Does that sound right to you?I would also have
blocks-autocompleters__user-slugandblocks-autocompleters__user-avatar.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have renamed the classes. I also tweaked where the completers' className is applied in the Autocompleter structure so their css won't care about the Autocompleter's class names.