Change dialog label for Quick Open based on mode #2298
Conversation
Added a switch case to set the dialogLabel correctly depending on which Navigation option is chosen.
Added a switch case to set the dialogLabel correctly depending on which Navigation option is chosen.
…into fix_issue_1705
…into sagarsane/fix_issue_1705
…er changes the first character. As part of this, changed some module functions so they're methods of QuickNavigateDialog instead, and added a module var to track the currently open dialog.
ghost
assigned 
|
@jasonsanjose - ping :) |
|
Oops, I missed this. I'll review today. |
| filter: _handleFilter, | ||
| resultFormatter: _handleResultsFormatter | ||
| filter: this._handleFilter, | ||
| resultFormatter: this._handleResultsFormatter |
There was a problem hiding this comment.
This use of this and that in showDialog() is a bit confusing now. On top of that, some of the event handlers are bound and others aren't: _handleItemSelect, _handleItemFocus, _handleKeyUp, _handleKeyDown, _handleResultsReady.
Also, just for consistency, I noticed _handleFilter and _handleResultsFormatter aren't wrapped in a wrapper function like the event handlers are. I'd actually prefer if we remove all the wrapper event handler functions (line 927-932) and change the signatures instead. It might be worth renaming _handleFilter and _handleResultsFormatter too since they aren't actually event handlers.
This is probably more refactoring than necessary, but I think at least we should fix the first issue with this/that and bind.
|
Initial review complete |
|
Code review changes pushed, merged with master. Ready for re-review. |
|
BTW, this code definitely could use some overall cleanup, but I'm hesitant to do a bunch more without unit tests. We should probably do it at the point where we work on the "official" Quick Open card. |
|
Looks good. Merging. |
Change dialog label for Quick Open based on mode
For #1705. Builds on @sagarsane's original pull request (#1706) to update the dialog label if the user changes the mode by editing the query string.
This ended up being a little complicated because we find out about changes to the query string through the
_handleFiltercallback to smart-autocomplete. This callback originally was just a module function, so didn't have the context of the dialog available. (We could have just directly looked up the ID in the global DOM, which other code in QuickOpen currently does, but it seems cleaner to actually look for it in the context of the dialog element.) So I changed this and some other related functions to be methods of QuickNavigateDialog instead.