Created attachment 598039 [details] [diff] [review] 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 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.