Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

3.0 api docs #2535

Merged
merged 7 commits into from
Apr 30, 2018
Merged

3.0 api docs #2535

merged 7 commits into from
Apr 30, 2018

Conversation

rictic
Copy link
Contributor

@rictic rictic commented Apr 25, 2018

Fixes #2379

Needs Polymer/tools#220 in order to produce this output, though we could merge this before that lands if we needed to.

package.json Outdated
@@ -11,7 +11,8 @@
"deploy": "scripts/deploy.sh",
"test": "(cd dist; python test_runner.py ${CLOUD_SDK-~/google-cloud-sdk} ../tests/)",
"generate-api-docs": "cd scripts && node generate_api_docs_2.js",
"generate-api-docs-1": "cd scripts && node generate_api_docs_1.js"
"generate-api-docs-1": "cd scripts && node generate_api_docs_1.js",
"generate-api-docs-3": "cd scripts && node generate_api_docs_3.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename generate-api-docs to generate-api-docs-2

@@ -0,0 +1,33 @@
// @ts-check
Copy link
Contributor

Choose a reason for hiding this comment

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

license


if (require.main === module) {
main().catch((e) => {
console.error(e ? e.stack || e.message || e: `Unknown error`);
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatting here makes this a little hard to parse

@@ -0,0 +1,501 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

license

const map = new Map();
/** @param {SimpleFeature} feature */
function index(feature, kind) {
const filename = getFilename(feature) || '???';
Copy link
Contributor

Choose a reason for hiding this comment

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

warn?

}

const hardcodedDescriptions = new Map([
['lib/elements/array-selector.js', `
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the way forward here? Can you open issues in Analyzer and Polymer to add these to the source?

const [feature] = features;
description = feature.summary;
}
description = description || hardcodedDescriptions.get(filename) || `TODO(write me): ${filename}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO or just warn?

Maybe these debug defaults should only be written with a flag on.

@rictic
Copy link
Contributor Author

rictic commented Apr 26, 2018

All comments addressed. Polymer/tools#241 is the path forward for file overviews, and instead of producing ??? or TODO in the final output, it throws instead.

@arthurevans
Copy link

A few thoughts:

Current file overview is a little confusing. Especially weird to have the tiny import line be the first thing on the page, followed by a giant headline for the element/class. This might look better once the @fileoverview tag is implemented.

Current format doesn't seem to document symbols that are re-exported. For example, the polymer-element Module page doesn't show the html function.

My first thought was that it was weird to have the element/class embedded directly in the module page , but I guess the vast majority of our modules only export one thing. The challenge here is that if a single module exported two elements, or an element and some functions, it would be virtually impossible to figure out that the other items were there unless you knew to look for them.

I think this issue could be addressed with a quick-reference at the beginning of the file, so you have something like:

File overview description

import {} from '@polymer/polymer/polymer-element.js'

Exports: PolymerElement, html, fooBar

Where each item in the Exports list is a link to the corresponding item.

But this might look weird if almost all of our files have only a single export.

A TOC or some other navigation aid would serve the same purpose of orienting people in the file (in fact, it'd be even better for large elements/mixins with lots of props and methods). Maybe we can get someone with a design-y eye to look at it.

As far as the URLs go, the current URL fragments assume that each module is only exporting a single class/element. For example, if you had a single module that exported simple-dialog and fancy-dialog, each with an open method, the fragments for both would look identical:

my-dialogs#method-open

Not sure if I am borrowing trouble here. But I'd hate to rewrite all of the links to the API docs and then change the URLs, so let's discuss this assumption.

Copy link

@arthurevans arthurevans left a comment

Choose a reason for hiding this comment

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

Left feedback on the main PR page about navigation and URL structure. I think we should discuss how to handle these before we launch. I don't think either of them needs to block merging of this PR. But we shouldn't rewrite the links to the API docs until we've discussed URL structure.

@arthurevans arthurevans merged commit 18276a2 into master Apr 30, 2018
@arthurevans
Copy link

Opened #2541 to track suggestions above.

@rictic rictic deleted the 3.0-api-docs branch April 30, 2018 23:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants