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

Doc Optimization #469

Merged
merged 7 commits into from
Feb 27, 2019
Merged

Doc Optimization #469

merged 7 commits into from
Feb 27, 2019

Conversation

patrickarlt
Copy link
Contributor

This PR proposes some of the changes I suggested in #137 (comment).

  1. On pages for functions like createItem
  2. Where there is a function param called requestOptions
  3. Print a table under "Options" with all the properties of the type of requestOptions.

Then to add some more value

  1. where the return type is a known declaration (i.e. something we have defined) in the library somewhere OR
  2. A promise that resolves to 1
  3. Print a table under "Returns" with all the properties of the type of the object.

This makes pages like createItem MUCH more useful since you can now reference all the options AND the return values on 1 page mostly resolving @dbouwman's issues in #137 (comment).

screen shot 2019-02-25 at 22 39 27-fullpage

@dbouwman
Copy link
Member

The output looks great @patrickarlt! The build is failing in CI though...

@jgravois
Copy link
Contributor

the build is failing because the postinstall script has been removed from the package.json.

if we don't want to bootstrap automatically on npm install anymore another option would be to add the command to the travis.yml as a before_script

@codecov
Copy link

codecov bot commented Feb 26, 2019

Codecov Report

Merging #469 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #469   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          94     94           
  Lines        1209   1209           
  Branches      219    219           
=====================================
  Hits         1209   1209
Impacted Files Coverage Δ
packages/arcgis-rest-geocoder/src/geocode.ts 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba893b7...d7a2772. Read the comment docs.

@patrickarlt
Copy link
Contributor Author

Fixed the build. I was having some issues related to npm install and lerna bootstrap

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

i'm pumped about this pull request @patrickarlt!

when you DRYed up the code in 7f58e72 you must have busted something, because the table documenting what is returned is no longer populated.

screenshot 2019-02-26 08 25 55

@patrickarlt
Copy link
Contributor Author

@jgravois whoops should be fixed now.

@patrickarlt patrickarlt requested a review from jgravois February 26, 2019 17:11
@COV-GIS
Copy link
Contributor

COV-GIS commented Feb 26, 2019

This is great @patrickarlt! The docs will be much more user friendly.

If you have time take a look at this... https://github.com/Esri/arcgis-rest-js/blob/master/docs/src/js/nav-toggle.js#L8-L12 matches both @esri/arcgis-rest-feature-service and @esri/arcgis-rest-feature-service-admin. So clicking @esri/arcgis-rest-feature-service opens both sub menus. I couldn't see a simple and elegant solution. Mildly annoying more than anything.

Copy link
Contributor

@noahmulfinger noahmulfinger left a comment

Choose a reason for hiding this comment

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

Looks great! Two notes:

  • I didn't do a full check of all pages, but I found a few pages that didn't show the Options table.

    • /arcgis-rest-js/api/users/getUserNotifications/
    • /arcgis-rest-js/api/users/getUser/
    • /arcgis-rest-js/api/users/getUserInvitations/
    • /arcgis-rest-js/api/feature-service/getLayer/
    • /arcgis-rest-js/api/groups/getGroup/
  • This is not blocking, but UI-wise its not super clear to me that the Options table refers to properties of the requestOptions param. Maybe just titling it "requestOptions Parameters" or indenting the Options table slightly would work. In any case, some sort of connection would be helpful.

@patrickarlt
Copy link
Contributor Author

patrickarlt commented Feb 27, 2019

@noahmulfinger I added support for when we need to handle things by named reference not just by ID. This resolves almost everything in your list.

However for arcgis-rest-js/api/users/getUser/ it accepts a union which is a BUNCH more work that I would rather delay. I might be able to get to it by dev summit, I might not.

As for the header this now says "Available requestOptions"

@jgravois jgravois dismissed noahmulfinger’s stale review February 27, 2019 16:50

feedback addressed.

@jgravois jgravois merged commit 6d2c6cf into master Feb 27, 2019
@jgravois
Copy link
Contributor

thank you @patrickarlt for the massive DX improvement and @noahmulfinger for the shrewd feedback!

@jgravois jgravois deleted the doc-optimization branch February 27, 2019 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants