Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Add option for first highlight index in Quick Open and Search History #13444

Merged
merged 4 commits into from
Jun 15, 2017
Merged

Add option for first highlight index in Quick Open and Search History #13444

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
71e60be
Fixed #13437
saurabh95 Jun 14, 2017
f72077a
minor change
saurabh95 Jun 14, 2017
80da659
Passed first highlight index as paramater to QuickSearchField
saurabh95 Jun 15, 2017
1ddc2ee
Added jsdocs
saurabh95 Jun 15, 2017
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
1 change: 1 addition & 0 deletions src/search/FindBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@ define(function (require, exports, module) {
this.searchField = new QuickSearchField(searchFieldInput, {
verticalAdjust: searchFieldInput.offset().top > 0 ? 0 : this._modalBar.getRoot().outerHeight(),
maxResults: 20,
shouldHighLightFirstIndex: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: shouldHighlightFirstIndex, the L in Light looks kinda unnatural

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we explicitly add this to the quick open field too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we should keep the highlighting for first element as default, and if someone wants to explicitly remove this he can pass false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sorry for the vague comment, I meant to explicitly add it as shouldHighLightFirstIndex: true in the Quick Open so it's defined in both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can do that

resultProvider: function (query) {
var asyncResult = new $.Deferred();
asyncResult.resolve(PreferencesManager.getViewState("searchHistory"));
Expand Down
13 changes: 12 additions & 1 deletion src/search/QuickSearchField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,13 @@ define(function (require, exports, module) {

$input.on("input", this._handleInput);
$input.on("keydown", this._handleKeyDown);

// For search History this flag is set to false
Copy link
Contributor

Choose a reason for hiding this comment

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

The flag is set to null for search History. As it's a numeric value, false doesn't make sense.

if (options.shouldHighLightFirstIndex === undefined) {
this._shouldHighLightFirstIndex = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just do

this._shouldHighLightFirstIndex === options.shouldHighLightFirstIndex || true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case if we pass false then it will taken as true, that's why I had to explicitly add the conditional statements

} else {
this._shouldHighLightFirstIndex = options.shouldHighLightFirstIndex;
}

this._dropdownTop = $input.offset().top + $input.height() + (options.verticalAdjust || 0);
}
Expand Down Expand Up @@ -277,7 +284,11 @@ define(function (require, exports, module) {
QuickSearchField.prototype._render = function (results, query) {
this._displayedQuery = query;
this._displayedResults = results;
this._highlightIndex = null;
if (this._shouldHighLightFirstIndex) {
this._highlightIndex = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I think of it, wouldn't it be easier to go with this._firstHighlightIndex, where the value would default to null but QuickOpen would have options._firstHighlightIndex set to 0?

} else {
this._highlightIndex = null;
}
// TODO: fixup to match prev value's item if possible?

if (results.error || results.length === 0) {
Expand Down