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.

Sign up for GitHub

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

Jump to bottom

Tags autocompleter keyboard interaction improvements. #8031

Merged
merged 3 commits into from
Aug 1, 2018
Merged

Tags autocompleter keyboard interaction improvements. #8031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24fe7c0
Tags autocompleter keyboard interaction improvements.
afercia Jul 18, 2018
80c6e9e
Keep entered text when pressing Escape.
afercia Jul 25, 2018
d8dc0e0
Remove showSuggestions in favor of isExpanded.
afercia Jul 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 45 additions & 21 deletions packages/components/src/form-token-field/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import classnames from 'classnames';
import { __, _n, sprintf } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { withInstanceId } from '@wordpress/compose';
import { BACKSPACE, ENTER, UP, DOWN, LEFT, RIGHT, SPACE, DELETE, ESCAPE } from '@wordpress/keycodes';

/**
* Internal dependencies
Expand Down Expand Up @@ -105,32 +106,36 @@ class FormTokenField extends Component {
let preventDefault = false;

switch ( event.keyCode ) {
case 8: // backspace (delete to left)
case BACKSPACE:
preventDefault = this.handleDeleteKey( this.deleteTokenBeforeInput );
break;
case 13: // enter/return
case ENTER:
preventDefault = this.addCurrentToken();
break;
case 37: // left arrow
case LEFT:
preventDefault = this.handleLeftArrowKey();
break;
case 38: // up arrow
case UP:
preventDefault = this.handleUpArrowKey();
break;
case 39: // right arrow
case RIGHT:
preventDefault = this.handleRightArrowKey();
break;
case 40: // down arrow
case DOWN:
preventDefault = this.handleDownArrowKey();
break;
case 46: // delete (to right)
case DELETE:
preventDefault = this.handleDeleteKey( this.deleteTokenAfterInput );
break;
case 32: // space
case SPACE:
if ( this.props.tokenizeOnSpace ) {
preventDefault = this.addCurrentToken();
}
break;
case ESCAPE:
preventDefault = this.handleEscapeKey( event );
event.stopPropagation();
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the use of stopPropagation here? Ideally omitting any mention of the word "autocompleter", as this is intended to be a general-use component unaware of any such context usage.

I'm not suggesting it's not needed, but I've become very wary of their use, as they have the tendency to be used as an extreme solution which are never obvious to evaluate as being necessary to the future maintainer. I might propose we go so far as to introduce an ESLint rule which we'd knowingly anticipate to be disabled in specific circumstances, but more as a general deterrent (also encouraging comments to explain the disabling).

break;
default:
break;
}
Expand Down Expand Up @@ -188,6 +193,9 @@ class FormTokenField extends Component {
const separator = this.props.tokenizeOnSpace ? /[ ,\t]+/ : /[,\t]+/;
const items = text.split( separator );
const tokenValue = last( items ) || '';
const inputHasMinimumChars = tokenValue.trim().length > 1;
const matchingSuggestions = this.getMatchingSuggestions( tokenValue );
const hasVisibleSuggestions = inputHasMinimumChars && !! matchingSuggestions.length;

if ( items.length > 1 ) {
this.addNewTokens( items.slice( 0, -1 ) );
Expand All @@ -202,11 +210,10 @@ class FormTokenField extends Component {

this.props.onInputChange( tokenValue );

const inputHasMinimumChars = tokenValue.trim().length > 1;
if ( inputHasMinimumChars ) {
const matchingSuggestions = this.getMatchingSuggestions( tokenValue );

this.setState( { isExpanded: !! matchingSuggestions.length } );
this.setState( {
isExpanded: hasVisibleSuggestions,
} );

if ( !! matchingSuggestions.length ) {
this.props.debouncedSpeak( sprintf( _n(
Expand Down Expand Up @@ -251,8 +258,16 @@ class FormTokenField extends Component {
}

handleUpArrowKey() {
this.setState( ( state ) => ( {
selectedSuggestionIndex: Math.max( ( state.selectedSuggestionIndex || 0 ) - 1, 0 ),
this.setState( ( state, props ) => ( {
selectedSuggestionIndex: (
( state.selectedSuggestionIndex === 0 ? this.getMatchingSuggestions(
state.incompleteTokenValue,
props.suggestions,
props.value,
props.maxSuggestions,
props.saveTransform
).length : state.selectedSuggestionIndex ) - 1
),
selectedSuggestionScroll: true,
} ) );

Expand All @@ -261,22 +276,31 @@ class FormTokenField extends Component {

handleDownArrowKey() {
this.setState( ( state, props ) => ( {
selectedSuggestionIndex: Math.min(
( state.selectedSuggestionIndex + 1 ) || 0,
this.getMatchingSuggestions(
selectedSuggestionIndex: (
( state.selectedSuggestionIndex + 1 ) % this.getMatchingSuggestions(
state.incompleteTokenValue,
props.suggestions,
props.value,
props.maxSuggestions,
props.saveTransform
).length - 1
).length
),
selectedSuggestionScroll: true,
} ) );

return true; // preventDefault
}

handleEscapeKey( event ) {
this.setState( {
incompleteTokenValue: event.target.value,
isExpanded: false,
selectedSuggestionIndex: -1,
selectedSuggestionScroll: false,
} );
return true; // preventDefault
}

handleCommaKey() {
if ( this.inputHasValidValue() ) {
this.addNewToken( this.state.incompleteTokenValue );
Expand Down Expand Up @@ -362,6 +386,7 @@ class FormTokenField extends Component {
incompleteTokenValue: '',
selectedSuggestionIndex: -1,
selectedSuggestionScroll: false,
isExpanded: false,
} );

if ( this.state.isActive ) {
Expand Down Expand Up @@ -510,6 +535,7 @@ class FormTokenField extends Component {
instanceId,
className,
} = this.props;
const { isExpanded } = this.state;
const classes = classnames( className, 'components-form-token-field', {
'is-active': this.state.isActive,
'is-disabled': disabled,
Expand All @@ -520,8 +546,6 @@ class FormTokenField extends Component {
tabIndex: '-1',
};
const matchingSuggestions = this.getMatchingSuggestions();
const inputHasMinimumChars = this.state.incompleteTokenValue.trim().length > 1;
const showSuggestions = inputHasMinimumChars && !! matchingSuggestions.length;

if ( ! disabled ) {
tokenFieldProps = Object.assign( {}, tokenFieldProps, {
Expand Down Expand Up @@ -549,7 +573,7 @@ class FormTokenField extends Component {
{ this.renderTokensAndInput() }
</div>

{ showSuggestions && (
{ isExpanded && (
<SuggestionsList
instanceId={ instanceId }
match={ this.props.saveTransform( this.state.incompleteTokenValue ) }
Expand Down
24 changes: 22 additions & 2 deletions packages/components/src/form-token-field/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,16 @@ describe( 'FormTokenField', function() {

describe( 'displaying tokens', function() {
it( 'should render default tokens', function() {
wrapper.setState( {
isExpanded: true,
} );
expect( wrapper.state.tokens ).toEqual( [ 'foo', 'bar' ] );
} );

it( 'should display tokens with escaped special characters properly', function() {
wrapper.setState( {
tokens: fixtures.specialTokens.textEscaped,
isExpanded: true,
} );
expect( getTokensHTML() ).toEqual( fixtures.specialTokens.htmlEscaped );
} );
Expand All @@ -137,13 +141,17 @@ describe( 'FormTokenField', function() {
// through unescaped to the HTML.
wrapper.setState( {
tokens: fixtures.specialTokens.textUnescaped,
isExpanded: true,
} );
expect( getTokensHTML() ).toEqual( fixtures.specialTokens.htmlUnescaped );
} );
} );

describe( 'suggestions', function() {
it( 'should not render suggestions unless we type at least two characters', function() {
wrapper.setState( {
isExpanded: true,
} );
expect( getSuggestionsText() ).toEqual( [] );
setText( 'th' );
expect( getSuggestionsText() ).toEqual( fixtures.matchingSuggestions.th );
Expand All @@ -157,40 +165,52 @@ describe( 'FormTokenField', function() {
} );

it( 'suggestions that begin with match are boosted', function() {
wrapper.setState( {
isExpanded: true,
} );
setText( 'so' );
expect( getSuggestionsText() ).toEqual( fixtures.matchingSuggestions.so );
} );

it( 'should match against the unescaped values of suggestions with special characters', function() {
setText( '& S' );
wrapper.setState( {
tokenSuggestions: fixtures.specialSuggestions.textUnescaped,
isExpanded: true,
} );
setText( '& S' );
expect( getSuggestionsText() ).toEqual( fixtures.specialSuggestions.matchAmpersandUnescaped );
} );

it( 'should match against the unescaped values of suggestions with special characters (including spaces)', function() {
setText( 's &' );
wrapper.setState( {
tokenSuggestions: fixtures.specialSuggestions.textUnescaped,
isExpanded: true,
} );
setText( 's &' );
expect( getSuggestionsText() ).toEqual( fixtures.specialSuggestions.matchAmpersandSequence );
} );

it( 'should not match against the escaped values of suggestions with special characters', function() {
setText( 'amp' );
wrapper.setState( {
tokenSuggestions: fixtures.specialSuggestions.textUnescaped,
isExpanded: true,
} );
expect( getSuggestionsText() ).toEqual( fixtures.specialSuggestions.matchAmpersandEscaped );
} );

it( 'should match suggestions even with trailing spaces', function() {
wrapper.setState( {
isExpanded: true,
} );
setText( ' at ' );
expect( getSuggestionsText() ).toEqual( fixtures.matchingSuggestions.at );
} );

it( 'should manage the selected suggestion based on both keyboard and mouse events', function() {
wrapper.setState( {
isExpanded: true,
} );
setText( 'th' );
expect( getSuggestionsText() ).toEqual( fixtures.matchingSuggestions.th );
expect( getSelectedSuggestion() ).toBe( null );
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/form-token-field/test/lib/token-field-wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,15 @@ class TokenFieldWrapper extends Component {
this.state = {
tokenSuggestions: suggestions,
tokens: Object.freeze( [ 'foo', 'bar' ] ),
isExpanded: false,
};
this.onTokensChange = this.onTokensChange.bind( this );
}

render() {
return (
<TokenField
suggestions={ this.state.tokenSuggestions }
suggestions={ this.state.isExpanded ? this.state.tokenSuggestions : null }
value={ this.state.tokens }
displayTransform={ unescapeAndFormatSpaces }
onChange={ this.onTokensChange }
Expand Down