Desktop: fix #3170: always make the first item selected when typing in GotoAnything #3181
Conversation
|
We need a description of what this pull request does. Also please give the issue a meaningful title, according to the template: https://github.com/laurent22/joplin/blob/master/.github/PULL_REQUEST_TEMPLATE |
ylc395
changed the title
ylc395
changed the title
| if (this.state.results.length > 0 && (keyCode === 40 || keyCode === 38)) { // DOWN / UP | ||
| if ((keyCode === 40 || keyCode === 38) && this.state.results.length > 0) { // DOWN / UP | ||
| event.preventDefault(); | ||
|
|
||
| const inc = keyCode === 38 ? -1 : +1; | ||
| let index = this.selectedItemIndex(); | ||
| if (index < 0) return; // Not possible, but who knows | ||
|
|
||
| index += inc; | ||
| if (index < 0) index = 0; | ||
| if (index >= this.state.results.length) index = this.state.results.length - 1; | ||
|
|
||
| const newId = this.state.results[index].id; | ||
| index = Math.min( | ||
| index + (keyCode === 38 ? -1 : +1), | ||
| this.state.results.length - 1, | ||
| ); |
There was a problem hiding this comment.
As I understand, the new code does the same as the previous one, so please undo these changes. It makes it easier to review pull request if we limit to only the necessary changes.
| let selectedItemId = null; | ||
| const itemIndex = this.selectedItemIndex(results, this.state.selectedItemId); | ||
| if (itemIndex > 0) { | ||
| selectedItemId = this.state.selectedItemId; | ||
| } else if (results.length > 0) { | ||
| selectedItemId = results[0].id; | ||
| } | ||
|
|
||
| this.itemListRef.current.makeItemIndexVisible(0); | ||
| this.setState({ | ||
| listType: listType, | ||
| results: results, | ||
| listType, | ||
| results, | ||
| resultsInBody, | ||
| selectedItemId: results.length === 0 ? null : results[0].id, | ||
| keywords: this.keywords(searchQuery), | ||
| selectedItemId: selectedItemId, | ||
| resultsInBody: resultsInBody, |
There was a problem hiding this comment.
I feel there's too many changes, and some of them aren't necessary. I feel the previous logic also made more sense.
There was a problem hiding this comment.
ok, I've converted some changes.
| selectedItemId = this.state.selectedItemId; | ||
| } else if (results.length > 0) { | ||
| selectedItemId = results[0].id; | ||
| } |
There was a problem hiding this comment.
no need to keep selectedItemId
|
Thanks for the update, but I don't know what bug is being fixed as I can't replicate it. Could you provide step by step instructions on how to replicate it? Also I think the selected item should be preserved between updates - see for example how it works in Sublime Text. |
|
key sequence: test↓ Look at scroll bar, after typing "test", I press down arrow, and it jumps to an un-excepted position(I think most users except the second item to be selected after typing down arrow), where is keeped since I typed "t". BTW: I hope you can give me some advice on #3180. I've added some tests. Anything else to be done? |
|
Ok that looks good too then, so let's merge. Thanks @ylc395! |
fix #3170. Whlie updating GotoAnything search input, the
selectedItemIndexwas reused between two updates, which caused that pressing Arrow Down key didn't select the second item. That made users confused. Fix this problem by ignoring the previousselectedItemIndexwhen updating GotoAnything search input.