728061 - Rework tutorial content, part 3

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: Documentation, defect, P1)

Description

[Will Bamberg [:wbamberg]]

Attachment: Pull Request 347

Pull request #347 introduces a lot of new, and reworked, tutorial content. This bug is to ask for review of the subset of them that talk about SDK tools and infrastructure.

Lots of these files are reworked from the original tutorial content, where I've tried to make them stand alone rather than being embedded in a longer story.

The files needing review for this bug are:
commonjs.md: https://github.com/mozilla/addon-sdk/pull/347/files#diff-5
getting-started-with-cfx.md: https://github.com/mozilla/addon-sdk/pull/347/files#diff-23
load-and-unload.md: https://github.com/mozilla/addon-sdk/pull/347/files#diff-26
logging.md: https://github.com/mozilla/addon-sdk/pull/347/files#diff-27
reusable-modules.md: https://github.com/mozilla/addon-sdk/pull/347/files#diff-31
unit-testing.md: https://github.com/mozilla/addon-sdk/pull/347/files#diff-33

Comment 1

[Brian Warner [:warner :bwarner]]

Comment on attachment 598039 [details] [diff] [review]
Pull Request 347


I reviewed just those 6 files: looks good! A few small mentions here:

* commonjs.md:
  - in the "we expect to make incompatible changes to them in future
    releases" bit near the end, you might expand this a bit, something like
    ".. which means, if you use these modules from your code, you may need to
    rewrite your code for future versions of the SDK", but with better words
    :-)

* getting-started-with-cfx.md
  - just add a newline to the end of the file

* load-and-unload.md:
  - you might mention that main() will be invoked a moment after the overall
    main.js is evaluated, and after all top-level require() statements have
    run (so generally after all dependent modules have been loaded).
  - the list of loadReason strings could be a <ul> instead of a <pre>.. I
    haven't looked at the rendered form to see which would look better, so no
    real preference either way. Giving the strings a monospaced font is
    probably a good idea.

* logging.md:
  - the sidebar about window.alert() confused me for a moment: "if you use it
    for diagnostics" makes it sound like you're about to describe what
    happens if you forget that alert() isn't available and use it anyways.
    Maybe say something like "window.alert isn't available, but console.log
    may serve as a useful replacement". Or "window.alert isn't available, so
    using it will cause an error, but console.log() is a useful replacement".

* reusable-modules.md
  - maybe say "you can split your code into separate modules" instead of
    using "factor your code" (I hear the word "refactor" used a lot, but not
    just plain "factor").
  - when packaging the new module, should the package.json have an 'id'? I
    think not. Do we need to mention this? I'm not sure.
  - looks like this file is missing a newline at EOF too

* unit-testing.md
  - the links to unit-test.html include fragments with inconsistent style:
    one uses #assertEqual(a, b, message) , and the next uses
    #assertRaises(func%2C predicate%2C message) . I don't know which is
    correct, or which (if either) will actually work to link to the specific
    assertion section in that other document.

Comment 2

[Will Bamberg [:wbamberg]]

Fixed by: https://github.com/mozilla/addon-sdk/commit/4e22d237650781a42874e95d3b8f9990a83c86cb.