-
Notifications
You must be signed in to change notification settings - Fork 13
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
F/search package #161
F/search package #161
Conversation
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.
hero's work @pranavkulkarni. this PR is really coming along.
besides my new comments, most everything in my first review still applies too.
packages/search/package.json
Outdated
@@ -0,0 +1,61 @@ | |||
{ | |||
"name": "@esri/hub-search", | |||
"version": "1.10.0", |
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.
1.11.1
(just so lerna will know to help us bump to v2.0.0
)
{ | ||
"name": "@esri/hub-search", | ||
"version": "1.10.0", | ||
"description": "Module to search for ArcGIS items and format them for display in ArcGIS Hub.", |
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.
it might be helpful to state explicitly that we're using this as a server-side module.
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.
actually, we will be using this on front end too.
test.html
Outdated
@@ -0,0 +1,78 @@ | |||
<!DOCTYPE html> |
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.
we don't want to check this one in.
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.
It is there in .gitignore. Not sure why it has appeared 😕
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.
i think you overrode the .gitignore at some point with an explicit git add test.html
.
luckily you can git rm test.html
too.
@@ -0,0 +1,9 @@ | |||
/* Copyright (c) 2018 Environmental Systems Research Institute, Inc. |
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.
2019
.
packages/search/src/ago/params.ts
Outdated
@@ -0,0 +1,29 @@ | |||
import { IRequestOptions } from "@esri/arcgis-rest-request"; | |||
|
|||
/* Copyright (c) 2017 Environmental Systems Research Institute, Inc. |
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.
2017
> 2019
(and move to the top of the file).
packages/search/src/ago/params.ts
Outdated
/* Copyright (c) 2017 Environmental Systems Research Institute, Inc. | ||
* Apache-2.0 */ | ||
|
||
export interface ISearchParams extends IRequestOptions { |
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.
ISearchOptions
(see #152 (comment))
|
||
const downloadableTypeKeywords = ["Data"]; | ||
// eligible types are listed here: http://doc.arcgis.com/en/arcgis-online/reference/supported-items.htm | ||
const downloadableTypes = [ |
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.
whether you keep this type in common
or search
, lets keep it DRY.
} | ||
|
||
// eligible types are listed here: http://doc.arcgis.com/en/arcgis-online/reference/supported-items.htm | ||
const downloadableTypes = [ |
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.
given what we already started in @esri/hub-common
, would it make more sense to export this type from there?
* require more complex and customized logic. Those go in the `customFacets` hash, | ||
* where the name of the key is the name of the facet being computed and the custom function | ||
* is implemented below. | ||
*/ |
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.
this code comment isn't being picked up by TSDoc.
for an example of method documentation that's formatted for display in our documentation, see:
hub.js/packages/annotations/src/add.ts
Lines 29 to 50 in b336f32
/** | |
* ```js | |
* import { addAnnotations } from "@esri/hub-annotations"; | |
* // | |
* addAnnotations({ | |
* url: annotationsUrl + "/0", | |
* features: [{ | |
* attributes: { | |
* target: "http://...", // required, explains what is being commented on | |
* description: "A grand idea!" // also required. this is the actual comment | |
* } | |
* }] | |
* }) | |
* .then(response); | |
* ``` | |
* Add an annotation to ArcGIS Hub. Uses authentication to derive authorship, appends a timestamp and sets a default status of "pending" to new comments by default. | |
* @param requestOptions - request options that may include authentication | |
* @returns A Promise that will resolve with the response from the service after attempting to add one or more new annotations. | |
*/ | |
export function addAnnotations( | |
requestOptions: IAddAnnotationsRequestOptions | |
): Promise<IAddFeaturesResult> { |
you can run npm run docs:serve
to build the documentation locally and see it update live as you make changes to the raw source.
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.
nice! I will fix that
* Construct a query object with filters, and queryParams sent by hub indexer | ||
* @param queryObject any | ||
*/ | ||
export function encodeAgoQuery(queryParams: any = {}) { |
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.
all export
ed functions show up in the documentation unless they are added to the explicit blacklist below.
i think it'd be appropriate to hide a lot of your shared utility functions, but i'll leave it up to you to decide exactly which.
Line 138 in da48fbc
const blacklist = []; |
const blacklist = [ "encodeAgoQuery", "etc" ];
now that v2.0.0 has shipped, we'll land this from #152 |
Module to search for ArcGIS items and format them for display in ArcGIS Hub
Structure --
Sample of how this will be used in UI code --
Sample of how hub indexer api v3 new search route will use hub.js --