-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
User autocomplete - use debounced search request #4406
Changes from 32 commits
52dc331
f357fce
6875efa
7afff73
badca63
6eb4432
8f57e6e
9dc23b9
ab425af
c67ef0d
00f9f0f
0edc3ff
c1e831c
bf356c0
1a6c124
874dce5
5f4206e
62ca388
e0d288f
510f642
135dc77
0806d95
e3d3f12
3cb5971
e9f76a1
dac63c0
6e3609b
2d703b5
98161e5
1dbba93
72486f5
36af45b
e4281a3
a14cbb0
eee4ce3
77dfe09
5e80481
895a5a4
654e7db
3202fb7
4649a30
2572de6
edb38af
70a4c1c
0bd2245
887b091
b9fd29a
b3b97ee
0432e65
ef79a0f
ac62c00
b15ab99
e216da5
2151b13
a0eedce
ea50097
8124f42
22bec2f
4be41af
d4f7a4c
ba2dcd3
2ecb819
ec7e31c
8d50225
4771c3e
a60ad7b
0023c9a
d4e063e
7b6904b
4f98c79
cdba191
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -221,9 +221,15 @@ export class Autocomplete extends Component { | |
} | ||
} | ||
|
||
loadOptions( index ) { | ||
this.props.completers[ index ].getOptions().then( ( options ) => { | ||
const keyedOptions = map( options, ( option, i ) => ( { ...option, key: index + '-' + i } ) ); | ||
/** | ||
* Load options for an autocompleter. | ||
* | ||
* @param {int} index The autocompleter index. | ||
* @param {string} query The query, if any. | ||
*/ | ||
loadOptions( index, query ) { | ||
this.props.completers[ index ].getOptions( query ).then( ( options ) => { | ||
const keyedOptions = map( options, ( option, i ) => ( { ...option, key: ( option.value.slug ? option.value.slug : index ) + '-' + i } ) ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you might have the key: index + '-' + ( option.value.slug ? option.value.slug : i ) This is because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, ok thanks for catching that - will correct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fixed this in eee4ce3 |
||
const filteredOptions = filterOptions( this.state.search, keyedOptions ); | ||
const selectedIndex = filteredOptions.length === this.state.filteredOptions.length ? this.state.selectedIndex : 0; | ||
this.setState( { | ||
|
@@ -312,7 +318,7 @@ export class Autocomplete extends Component { | |
} | ||
|
||
search( event ) { | ||
const { open: wasOpen, suppress: wasSuppress } = this.state; | ||
const { open: wasOpen, suppress: wasSuppress, query: wasQuery } = this.state; | ||
const { completers } = this.props; | ||
const container = event.target; | ||
// ensure that the cursor location is unambiguous | ||
|
@@ -324,8 +330,8 @@ export class Autocomplete extends Component { | |
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 ); | ||
if ( open && ( ! wasOpen || open.idx !== wasOpen.idx || query !== wasQuery ) ) { | ||
this.loadOptions( open.idx, query ); | ||
} | ||
// create a regular expression to filter the options | ||
const search = open ? new RegExp( '(?:\\b|\\s|^)' + escapeRegExp( query ), 'i' ) : /./; | ||
|
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.
Returning a promise that may never resolve makes me uncomfortable as I am unsure if it will cause memory to leak (it probably won't but it doesn't hurt to avoid the situation). I suggest that you could cause the promise to reject with a specific error (just use a string) that you can check for in
loadOptions
.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.
yea, i have to agree - I feel the same way. unfortunately, this was the only way I could get the debouncing to work and part of why i had moved the debouncing up a level. debouncing a function that returns a promise is a bit problematic.
ideally, we should debounce the function that calls the promise returning function - that way no extra promises are created. let me revisit the idea of moving this "up" again is a way that doesn't affect other autocompleters.
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.
@EphoxJames I tried a different approach in eee4ce3
I've added an attribute
isDebouncedCompleter: true
to the object returned byuserAutocompleter
. The in the autocomplete component, when this is set for an autocompleter, I calldebouncedLoadOptions
to debounce the async callbacks. This lets any autocompleter opt in to debouncing.Let me know what you think of this approach.