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.

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

Search Box Autocomplete #5424

Merged
merged 13 commits into from
Feb 20, 2018
Merged

Search Box Autocomplete #5424

merged 13 commits into from
Feb 20, 2018

Conversation

ryuyu
Copy link
Contributor

@ryuyu ryuyu commented Feb 9, 2018

This PR adds in search box "autocomplete" and a new api endpoint to support this. @anangaur
On Front Page:
image

On Packages Page:
image

Current interaction scheme:
Arrow keys up and Down while focused in search box will move up and down the autocomplete list.
Tabbing/esc will close the list.
Enter with result selected/focused will navigate to package details page.

@anangaur
Copy link
Member

anangaur commented Feb 9, 2018

Nice!! I like black color better than the blue for the package name. Is it possible to append the download count along with the package ID?

E.g.

MyPackage.Sample (<downloadIcon>21,345) Description.

Package ID - black
Rest - Gray

Copy link
Contributor

@scottbommarito scottbommarito left a comment

Choose a reason for hiding this comment

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

Looks good from what I saw but I also am not particularly knowledgeable about the JS stuff.

return;
}

var requestUrl = "/api/v2/package-ids?partialId=" + currInput + "&semVerLevel=2.0.0";
Copy link
Contributor

Choose a reason for hiding this comment

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

If you can, try to pass in this URL template in using some method on UrlExtensions like we do with some of our other scripts. Same with the package-details URL below. Not a big deal tho.

});

$("#autocomplete-results-container").keydown(function (e) {
if (e.keyCode === 40) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think every keycode should have either a comment or an appropriately named constant. You put comments in the searchBox key which was good, but I don't see comments on all the keycodes.

if (e.KeyCode === _downArrow) {

or

if (e.KeyCode === 40) { // down arrow


function hookAutocomplete(maxResults) {
_resultsCache.results = ko.observable();
$(document).keydown(function (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to add some more comments through this code in general, we're C# devs not JS devs so some of this stuff goes over our heads a little more than the usual knockout stuff does.

Especially for the stuff below with the timeouts, e.g.

// We will only begin loading the autocomplete results X seconds after the user has typed into the autocomplete box.

@@ -686,6 +686,26 @@ protected internal virtual Stream ReadPackageFromRequest()
};
}

[HttpGet]
[ActionName("PackageDetails")]
public virtual async Task<ActionResult> GetPackageDetails(
Copy link
Contributor

Choose a reason for hiding this comment

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

GetPackageDetails [](start = 48, length = 17)

change the name to indicate that this searches

return;
}

var requestUrl = "/api/v2/package-ids?partialId=" + currInput + "&semVerLevel=2.0.0";
Copy link
Contributor

Choose a reason for hiding this comment

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

please test on DEV to see if this works well on APIM

var tempId = idList[i];
var searchData = _resultsCache[safeId(tempId)];
if (typeof searchData() == "string") {
requestUrl += "packageId:" + idList[i] + " ";
Copy link
Member

Choose a reason for hiding this comment

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

nit: packageid

}

function setupAllAuxData(idList) {
var requestUrl = "/api/v2/package-details?searchString=";
Copy link
Member

Choose a reason for hiding this comment

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

q

Copy link
Member

Choose a reason for hiding this comment

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

(instead of searchString

@joelverhagen
Copy link
Member

Have we done an accessibility pass on this?

Could you ask @anangaur if we should talk to UX exports about this change?

@joelverhagen
Copy link
Member

Accessibility then :shipit:

@maartenba
Copy link
Contributor

A new endpoint under /api/v2. Can it somehow be documented? Do third party servers need it?

@joelverhagen
Copy link
Member

@maartenba, this endpoint is an implementation detail for the auto-complete box and will not be used by the client.

Can it somehow be documented?

@ryuyu, @skofman1 - feel free to chime in here, but seeing as even the parts of the V2 protocol that the client uses are not documented, it doesn't make sense here.

Do third party servers need it?

Nope. Unless you want to implement an auto-complete search box in the same way... but even that it's an implementation detail.

@anangaur
Copy link
Member

I am not in favor of documenting any V2 APIs. If required, we can have a work item tracking a corresponding V3 API but not something we can work on, in near term.

@maartenba
Copy link
Contributor

Thanks! Not a requirement imo but given it suddenly appears on the API/v2 path I just had to ask.

@xavierdecoster
Copy link
Member

@anangaur @joelverhagen please bare in mind that properly rolling out APIM in front of the gallery may require "some" v2 API docs going forward.

Reason: APIM efforts mandate we define these API endpoints, and their expected behavior in APIM config rules (e.g. request throttling, redirect to HTTPS, etc).
Missing an endpoint under /api/v2 in APIM config rules can have unintended side-effects.
On the other hand, the very act of configuring APIM rules in a way is documenting the API v2 endpoints ;)

@ryuyu ryuyu merged commit ae89732 into dev Feb 20, 2018
@ryuyu ryuyu deleted the ryuyu-autocomplete branch March 21, 2019 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants