Polished first pass on property value REST docs #95

KilimAnnejaro · 2020-11-15T02:58:02Z

First pass on content rewrite for the property value REST API docs. Comments:

I was unsure of the explanatory verbiage for the direction field--feedback, suggestions?
I formatted the JSON responses to look pretty, in the process changing some of the structure of the response. (For example, the strings returned are represented as JSON objects without backslash characters.) Let me know if you'd like me to change this.

Thank you!

This change is

beets

Thanks Anne, this is really shaping up! I like the structure and flow of the page.

I commented inline about the 'direction' field (which was a source of confusion for me when I started).

Regarding the JSON responses - I think it's also a source of confusion and we shouldn't change the structure of the response so that users aren't thrown off when they try to replicate the examples.

I also wonder if we can be clearer with examples, especially on how to get at the result you're looking for when you do a query? Especially for the simpler cases, where e.g. "get me the name of geoId/06" -- how do I go from response to result (California). We were asked quite explicitly for examples, which is why we added the JSFiddle, though that was only in JS, and we could add examples for other methods too (cmd-line, etc)

api/rest/property_value.md

beets · 2020-11-17T19:39:38Z

api/rest/property_value.md

-    ```bash
-    curl 'https://api.datacommons.org/node/property-values?dcids=geoId/05&property=containedInPlace&direction=out'
-    ```
+  - `direction`: You can specify this argument as `out` to indicate that you desire the response to only include nodes which are supercategories of the specified `DCIDs`, or `in` to only return nodes that are subcategories of the specified `DCIDs`. (For example, South America is a supercategory of Argentina, which in turn is a supercategory of Buenos Aires.)


I think this is a good example. I wonder if it would be even clearer with a small node image, with pointed arrows to indicate direction, and some example queries (i.e. how does a user translate that example into property value queries?)

I added an image next to Example 5, the query that determines the country in which Buenos Aires is located. Does that location seem good to you? Furthermore, would you like for me to also add queries for the other three arrows on the diagram (ie, the continent on which Argentina is located, countries on the continent of South America, and also cities/administrative areas within Argentina)?

api/rest/property_value.md

tjann

Thanks for adding all the examples!

api/rest/property_value.md

tjann · 2020-11-17T20:00:35Z

api/rest/property_value.md


 ```json
 {
    "payload": "<payload string>",
 }
 ```

-The equivalent JSON of "payload string" is as follows.
+Here `"<payload string>"` is replaced by JSON, whose structure changes depending on whether the response contains node references.


We can consider adding/linking to documentation that talks about how there are only 2 values in DC: references and primitives.

…er decoding

beets

Thanks for adding the tabs, in general this is looking good!

Reviewable status: 0 of 8 files reviewed, 24 unresolved discussions (waiting on @beets, @KilimAnnejaro, @shifucun, and @tjann)

_layouts/default.html, line 11 at r5 (raw file):

  <link href="https://fonts.googleapis.com/css2?family=Public+Sans:wght@600&family=Roboto&display=swap" rel="stylesheet">
  <link href="/assets/css/styles.css" rel="stylesheet">
  <link rel="stylesheet" href="/assets/css/custom.css">

We shouldn't add additional css downloads. See comment in custom.css for a pointer to add it to the styles.css bundle

api/rest/property_value.md, line 43 at r1 (raw file):

Previously, beets wrote…

Nit: maybe we shouldn't specify "four" if the number of arguments changes.

Maybe: Here are helpful arguments ... (to avoid forgetting to update the count in the future when this list changes)

api/rest/property_value.md, line 47 at r1 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

I added an image next to Example 5, the query that determines the country in which Buenos Aires is located. Does that location seem good to you? Furthermore, would you like for me to also add queries for the other three arrows on the diagram (ie, the continent on which Argentina is located, countries on the continent of South America, and also cities/administrative areas within Argentina)?

That looks great! Yes I think adding the property that corresponds to each edge will be helpful (and we should point to the example below that uses this diagram)

api/rest/property_value.md, line 73 at r1 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

I've added language to indicate the hard decoding requirement.

great, thanks!

api/rest/property_value.md, line 30 at r5 (raw file):

valued as

Maybe: Only valid values are ... or Only values allowed are ...

api/rest/property_value.md, line 73 at r5 (raw file):

{
    "dcid": {
        "direction": [

We should try to differentiate between when it's the actual key vs when it's a string that contains a specified value, e.g. top level "dcid" is actually the dcid you queried for, vs "dcid" in the innermost object is the key "dcid". Maybe "" and "dcid"? Thoughts?

api/rest/property_value.md, line 140 at r5 (raw file):

js

Should this be "JavaScript"?

api/rest/property_value.md, line 329 at r5 (raw file):

earthquakes

Nit: earthquake events

api/rest/property_value.md, line 404 at r5 (raw file):

cyclone

Nit: cyclone event

assets/css/custom.css, line 1 at r5 (raw file):

.tab {

Perhaps this file should be renamed to tabs.scss (and we can import it into styles.scss - jekyll will compile it into one css module for serving)

assets/images/SAmWithDir.png, line 0 at r5 (raw file):
Can we add a new subdirectory for this image (assets/images/rest) and follow the existing naming scheme?
e.g. property_value_direction_example.png

I wonder if it would be helpful to also add the property corresponding to each edge.

assets/js/tabs.js, line 1 at r5 (raw file):

const removeActiveClasses = function (ulElement) {

Please add a top-level comment describing the contents of this module and each function.

assets/js/tabs.js, line 6 at r5 (raw file):

        li.classList.remove('active');
    });
  }

Please watch the indentation for here and the following function

assets/js/tabs.js, line 10 at r5 (raw file):

  const getChildPosition = function (element) {
        var parent = element.parentNode;
        var i = 0;

This line isn't necessary since i is redefined in the for loop

assets/js/tabs.js, line 16 at r5 (raw file):

            }
        }

Maybe add a comment that this should not be reachable?

assets/js/tabs.js, line 21 at r5 (raw file):

window.addEventListener('load', function () {
    const tabLinks = document.querySelectorAll('ul.tab li a');

I think the code would be simpler if we installed the onclick on the li element.

assets/js/tabs.js, line 30 at r5 (raw file):

        ulTab = liTab.parentNode;
        position = getChildPosition(liTab);
        if (liTab.className.includes('active')) {

Maybe move this two lines up (after liTab). Since ulTab and position isn't necessary if we early return.

assets/js/tabs.js, line 39 at r5 (raw file):

        removeActiveClasses(tabContentElement);

        tabContentElement.querySelectorAll('li')[position].classList.add('active');

Perhaps I'm missing something, but is this the equivalent of: liTab.classList.add('active') (we might not need position after all)

beets · 2020-11-24T18:54:30Z

I apologize, I should've mentioned that I'm testing out using reviewable.io for the PR review. I thought it would have synced the comments inline with the file viewer here. We can keep things in github PR if you prefer, let me know!

beets · 2020-11-25T23:01:51Z

Could we try reviewable for this PR at least? We were finding that we missed out on unresolved comments when the PR's get large, and this at least highlights which are left unresolved.

beets · 2020-11-25T23:05:39Z

Agree though that the content looks great! Just a few more things to resolve. Thanks for working through this with us Anne!

KilimAnnejaro

Thank you!

Reviewable status: 3 of 7 files reviewed, 10 unresolved discussions (waiting on @beets, @KilimAnnejaro, and @tjann)

api/rest/property_value.md, line 47 at r1 (raw file):

Previously, beets wrote…

Hm, after looking at this more, can we use it in terms of containedInPlace? i.e. for the property containedInPlace on the node "Argentina", in results in Bueno Aires, out results in South America. And we could simplify that figure with just those two edges, with the containedInPlace label (since in/out is denoted by the arrow).

I am not sure I understand what I should do about the diagram? But I did update this description to use the containedInPlace language rather than the supercategory/subcategory bits.

api/rest/property_value.md, line 73 at r5 (raw file):

Previously, beets wrote…

I think works! Also note that it should be . We can point to the example responses below.

I don't understand?

api/rest/property_value.md, line 140 at r5 (raw file):

Previously, beets wrote…

I think JavaScript is more accurate...

Yep, we can do that!

api/rest/property_value.md, line 38 at r6 (raw file):

Previously, beets wrote…

We should also try avoiding counting the arguments, though keeping "requires two arguments" seems reasonable.

Done.

api/rest/property_value.md, line 42 at r6 (raw file):

Previously, beets wrote…

I think @tjann was suggesting adding some possible properties in the description.

So I just checked on Github and it appears to me that I incorporated @tjann 's comment verbatim into the PR. Am I looking at the wrong page?

api/rest/property_value.md, line 24 at r7 (raw file):

Previously, tjann (TJann) wrote…

I might suggest removing "label"

Done.

api/rest/property_value.md, line 109 at r7 (raw file):

Previously, tjann (TJann) wrote…

Retrieve the common name(s) ? I don't want users to think we are doing limit=2

Done.

assets/js/tabs.js, line 1 at r5 (raw file):

Previously, beets wrote…

Thanks! Could you move the docs for the functions to right above each function?

Yep, this is a good idea!

beets

Sorry for the late response on the PR!

Reviewable status: 3 of 7 files reviewed, 10 unresolved discussions (waiting on @beets, @KilimAnnejaro, and @tjann)

api/rest/property_value.md, line 47 at r1 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

I am not sure I understand what I should do about the diagram? But I did update this description to use the containedInPlace language rather than the supercategory/subcategory bits.

How about something like this:

direction: This refers to the orientation, or direction, of the edge. You can specify this argument as in or out. (For example, the figure below illustrates the directions for the property containedInPlace of the node for Argentina).

[ South America ] <-- containedInPlace -- [ Argentina ] <-- containedInPlace -- [ Buenos Aires ]

PropertyValue(containedInPlace, Argentina, 'in') --> Buenos Aires
PropertyValue(containedInPlace, Argneinta, 'out') --> South America

(not sure if that pseudocode adds to the confusion.. but something to that effect?)

api/rest/property_value.md, line 73 at r5 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

I don't understand?

Huh, maybe it was lost with the formatting. I meant to say, for direction -- it should also have the angle brackets since it will have the values "in" or "out".

api/rest/property_value.md, line 140 at r5 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

Yep, we can do that!

Update to "JavaScript" (capitalized J and S)?

api/rest/property_value.md, line 42 at r6 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

So I just checked on Github and it appears to me that I incorporated @tjann 's comment verbatim into the PR. Am I looking at the wrong page?

Ah looks like it was added in the next commit. Thanks

api/rest/property_value.md, line 479 at r9 (raw file):

Example 6: Retrieve the country in which Buenos Aires is located.

Took a read over of the examples again and I think we should update this to match the figure (which we should include here and above with the "direction" explanation)

Can we do this as the query for this example:
https://api.datacommons.org/node/property-values?dcids=country/ARG&property=containedInPlace

Note how there are results for both 'in' and 'out, which I think highlights the directionality well.

assets/images/SAmWithDir.png, line at r5 (raw file):
Could this be a good diagram?

[ South America ] <-- containedInPlace -- [ Argentina ] <-- containedInPlace -- [ Buenos Aires ]

KilimAnnejaro

no worries 😃

Reviewable status: 3 of 7 files reviewed, 6 unresolved discussions (waiting on @beets and @KilimAnnejaro)

api/rest/property_value.md, line 47 at r1 (raw file):

Previously, beets wrote…

How about something like this:

direction: This refers to the orientation, or direction, of the edge. You can specify this argument as in or out. (For example, the figure below illustrates the directions for the property containedInPlace of the node for Argentina).

[ South America ] <-- containedInPlace -- [ Argentina ] <-- containedInPlace -- [ Buenos Aires ]

PropertyValue(containedInPlace, Argentina, 'in') --> Buenos Aires
PropertyValue(containedInPlace, Argneinta, 'out') --> South America

(not sure if that pseudocode adds to the confusion.. but something to that effect?)

So I went ahead and English-ized the pseudocode into the caption and also modified the image as we discussed. I don't love the way the caption is worded, but I think it hits the main ideas.

api/rest/property_value.md, line 73 at r5 (raw file):

Previously, beets wrote…

Huh, maybe it was lost with the formatting. I meant to say, for direction -- it should also have the angle brackets since it will have the values "in" or "out".

Done.

api/rest/property_value.md, line 140 at r5 (raw file):

Previously, beets wrote…

Update to "JavaScript" (capitalized J and S)?

Done.

assets/images/SAmWithDir.png, line at r5 (raw file):

Previously, beets wrote…

Could this be a good diagram?

[ South America ] <-- containedInPlace -- [ Argentina ] <-- containedInPlace -- [ Buenos Aires ]

Changed the diagram and incorporated the suggestions!

KilimAnnejaro

Reviewable status: 3 of 7 files reviewed, 6 unresolved discussions (waiting on @beets)

api/rest/property_value.md, line 479 at r9 (raw file):

Previously, beets wrote…

Took a read over of the examples again and I think we should update this to match the figure (which we should include here and above with the "direction" explanation)

Can we do this as the query for this example:
https://api.datacommons.org/node/property-values?dcids=country/ARG&property=containedInPlace

Note how there are results for both 'in' and 'out, which I think highlights the directionality well.

I've updated the query in the most recent commit.
I am a little concerned about repeating the figure here as well as at the top, since the page seems too short for such a repetition. However, I'm not sure--thoughts?

beets

Reviewable status: 3 of 7 files reviewed, 4 unresolved discussions (waiting on @beets, @KilimAnnejaro, and @tjann)

api/rest/property_value.md, line 47 at r1 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

So I went ahead and English-ized the pseudocode into the caption and also modified the image as we discussed. I don't love the way the caption is worded, but I think it hits the main ideas.

With the suggestion in my last comment, I also wanted to clarify that this doesn't only apply to "containedInPlace" -- this applies to all properties (the second sentence of the description). Could you update that as well?

api/rest/property_value.md, line 73 at r5 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

Done.

Could you apply it to Structure 1 as well?

api/rest/property_value.md, line 140 at r5 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

Done.

Thanks!

api/rest/property_value.md, line 479 at r9 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

I've updated the query in the most recent commit.
I am a little concerned about repeating the figure here as well as at the top, since the page seems too short for such a repetition. However, I'm not sure--thoughts?

I think it's ok - the page is long with the example blocks. But if we wanted to include it only once, I think it belongs above, with the explanation of "direction"

api/rest/property_value.md, line 483 at r10 (raw file):

![](/assets/images/rest/property_value_direction_example.png)

*Figure 1. Relationship diagram for the property `containedInPlace` of the country of Argentina, for which this property takes on the value `South America`, and the city of Buenos Aires, for which this property takes on the value `Argentina`.*

What we want to highlight here is the directionality of containedInPlace. How about this:

Relationship diagram for the property containedInPlace of the country Argentina. Note the directionality of the property containedInPlace: the API returns both in (Buenos Aires is containedInPlace of Argentina), as well as out (Argentina is containedInPlace of South America).

KilimAnnejaro

Reviewable status: 3 of 7 files reviewed, 4 unresolved discussions (waiting on @beets)

api/rest/property_value.md, line 47 at r1 (raw file):

Previously, beets wrote…

With the suggestion in my last comment, I also wanted to clarify that this doesn't only apply to "containedInPlace" -- this applies to all properties (the second sentence of the description). Could you update that as well?

Done.

api/rest/property_value.md, line 73 at r5 (raw file):

Previously, beets wrote…

Could you apply it to Structure 1 as well?

Done 😃

api/rest/property_value.md, line 479 at r9 (raw file):

Previously, beets wrote…

I think it's ok - the page is long with the example blocks. But if we wanted to include it only once, I think it belongs above, with the explanation of "direction"

I added it a second time. If we think that's too much, no problem--I can always just remove one of the image instances!

api/rest/property_value.md, line 483 at r10 (raw file):

Previously, beets wrote…

What we want to highlight here is the directionality of containedInPlace. How about this:

Relationship diagram for the property containedInPlace of the country Argentina. Note the directionality of the property containedInPlace: the API returns both in (Buenos Aires is containedInPlace of Argentina), as well as out (Argentina is containedInPlace of South America).

YES this is exactly right! I made a few small changes to the wording and text styling, but this is much closer than where we were before.

beets

Reviewable status: 3 of 7 files reviewed, 2 unresolved discussions (waiting on @beets)

api/rest/property_value.md, line 483 at r10 (raw file):

Previously, KilimAnnejaro (Anne E. Ulrich) wrote…

YES this is exactly right! I made a few small changes to the wording and text styling, but this is much closer than where we were before.

Nice, thanks!

beets · 2020-12-03T21:45:48Z

Thanks for working through this! Let's get this in :)

Builds site and commits static files to gh-pages branch on every commit to master branch. Github pages only allows a small set of plugins. We recently added jekyll-tabs in #95, and will probably add more in the future. If this action works well, will update the site settings to serve what is in gh-pages branch.

polished first pass on property value rest docs

605d4fb

KilimAnnejaro requested review from beets and tjann November 15, 2020 02:58

KilimAnnejaro added 3 commits November 15, 2020 11:27

cleanup

814838d

word

a6d0c2d

thought of a better example

8c0b275

beets reviewed Nov 17, 2020

View reviewed changes

tjann reviewed Nov 17, 2020

View reviewed changes

KilimAnnejaro added 14 commits November 17, 2020 20:03

ellipsis :-)

0d6809b

sentence case after colon!

68e5982

no need for two 4s

493c3fa

image for property value supercategories

3d38cf5

bashing on curl

53202bd

fixes per tjann

be5ca26

removing unneeded verbiage

f87fd16

making it very clear that the pretty structures are only possible aft…

5f9d7b4

…er decoding

merged notes into main text body

12d635e

styling and tabs

0dad631

file reorg

4f78f66

rename

f885884

appropriate tabs for requests

0932e4f

cleanup--js fiddle, name example

401b786

KilimAnnejaro requested review from beets and tjann November 24, 2020 04:48

beets reviewed Nov 24, 2020

View reviewed changes

KilimAnnejaro added 4 commits November 24, 2020 21:02

direction term

e0b9662

two

f4b26ec

<>

3f3fab0

earthquake events

536143c

KilimAnnejaro added 5 commits November 25, 2020 19:55

removing word

81430b0

removing two

ee328db

js -> javascript

bd050bf

moving comments around

96857c0

supercategory -> containedInPlace

c26a847

KilimAnnejaro commented Nov 26, 2020

View reviewed changes

KilimAnnejaro requested a review from beets November 28, 2020 19:18

beets requested changes Dec 1, 2020

View reviewed changes

KilimAnnejaro added 3 commits December 1, 2020 17:52

javascript -> JavaScript

d71f315

changes per feedback

b970d83

<direction>

bc7938b

KilimAnnejaro commented Dec 2, 2020

View reviewed changes

buenos aires -> argentina

f2d1119

KilimAnnejaro commented Dec 2, 2020

View reviewed changes

KilimAnnejaro requested a review from beets December 2, 2020 04:22

beets requested changes Dec 2, 2020

View reviewed changes

KilimAnnejaro added 4 commits December 2, 2020 19:05

general properties

8515e07

<direction>

d69093c

repeating the image

22c6f1a

caption improvements

8d36c98

KilimAnnejaro commented Dec 3, 2020

View reviewed changes

KilimAnnejaro requested a review from beets December 3, 2020 01:13

beets approved these changes Dec 3, 2020

View reviewed changes

Merge branch 'master' into rest-property-value-content

b814e20

beets merged commit e71c06e into datacommonsorg:master Dec 3, 2020

beets mentioned this pull request Dec 7, 2020

Create github-pages.yml #109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polished first pass on property value REST docs #95

Polished first pass on property value REST docs #95

KilimAnnejaro commented Nov 15, 2020 •

edited by beets

Loading

beets left a comment

beets Nov 17, 2020

KilimAnnejaro Nov 18, 2020

tjann left a comment

tjann Nov 17, 2020

beets left a comment

beets commented Nov 24, 2020

beets commented Nov 25, 2020

beets commented Nov 25, 2020

KilimAnnejaro left a comment

beets left a comment

KilimAnnejaro left a comment

KilimAnnejaro left a comment

beets left a comment

KilimAnnejaro left a comment

beets left a comment

beets commented Dec 3, 2020

Polished first pass on property value REST docs #95

Polished first pass on property value REST docs #95

Conversation

KilimAnnejaro commented Nov 15, 2020 • edited by beets Loading

beets left a comment

Choose a reason for hiding this comment

beets Nov 17, 2020

Choose a reason for hiding this comment

KilimAnnejaro Nov 18, 2020

Choose a reason for hiding this comment

tjann left a comment

Choose a reason for hiding this comment

tjann Nov 17, 2020

Choose a reason for hiding this comment

beets left a comment

Choose a reason for hiding this comment

beets commented Nov 24, 2020

beets commented Nov 25, 2020

beets commented Nov 25, 2020

KilimAnnejaro left a comment

Choose a reason for hiding this comment

beets left a comment

Choose a reason for hiding this comment

Example 6: Retrieve the country in which Buenos Aires is located.

KilimAnnejaro left a comment

Choose a reason for hiding this comment

KilimAnnejaro left a comment

Choose a reason for hiding this comment

beets left a comment

Choose a reason for hiding this comment

KilimAnnejaro left a comment

Choose a reason for hiding this comment

beets left a comment

Choose a reason for hiding this comment

beets commented Dec 3, 2020

KilimAnnejaro commented Nov 15, 2020 •

edited by beets

Loading