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

docs: Add summary to docs and fix command descriptions #1062

Merged
merged 2 commits into from
Aug 25, 2017

Conversation

kumar303
Copy link
Contributor

The command descriptions now follow the new extension terminology: https://wiki.mozilla.org/Add-ons/Terminology

The command descriptions now follow the new extension terminology: https://wiki.mozilla.org/Add-ons/Terminology
@kumar303 kumar303 requested a review from rpl August 25, 2017 17:09
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5f86c0c on kumar303:update-docs into b0250ed on mozilla:master.

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@kumar303 looks good to me (I only have two comments related to small optional nits).

README.md Outdated
@@ -17,6 +17,19 @@ cross-platform way. Initially, it will provide a streamlined experience for deve
* [Getting started with web-ext](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Getting_started_with_web-ext)
* [Command reference](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/web-ext_command_reference)

Here are the commands you can run. Click on each one for detailed documentation or use `--help` on the command line, such as `web-ext --help build`.
Copy link
Member

Choose a reason for hiding this comment

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

nit, not a big deal (and I'm pretty sure that with yargs the order doesn't matter), but as a command line user I'm more used to see the opposite web-ext build --help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. That's more intuitive too since you may start typing web-ext run ... and then realize you don't know the name of some option.

README.md Outdated
@@ -17,6 +17,19 @@ cross-platform way. Initially, it will provide a streamlined experience for deve
* [Getting started with web-ext](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Getting_started_with_web-ext)
* [Command reference](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/web-ext_command_reference)

Here are the commands you can run. Click on each one for detailed documentation or use `--help` on the command line, such as `web-ext --help build`.

* [`build`](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/web-ext_command_reference#web-ext_build)
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it would be better to order them based on how much they are actually used (and useful) or just ordering them alphabetically.

If we opt for ordering them by "number of times you will actually use the command", I would say:

  • run, build, lint, sign, docs
    or
  • run, lint, build, sign, docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. The first ordering is what I would think. I'll change it to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, actually, build really isn't so useful. This comes to mind after more thought: run, lint, sign, build, docs

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 061fa46 on kumar303:update-docs into b0250ed on mozilla:master.

@kumar303 kumar303 merged commit 1cdecfb into mozilla:master Aug 25, 2017
@kumar303 kumar303 deleted the update-docs branch August 25, 2017 17:31
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.

None yet

3 participants