Issue 29325800: Noissue - Improve CMS usage documentation (Closed)
|
Created:
Sept. 3, 2015, 11:54 a.m. by kzar Modified:
Sept. 3, 2015, 3:10 p.m. CC:
saroyanm Visibility:
Public. |
DescriptionNoissue - Improve CMS usage documentation
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed Wladimir's feedback #
Total comments: 4
Patch Set 3 : Addressed further feedback #MessagesTotal messages: 6
Patch Set 1
https://codereview.adblockplus.org/29325800/diff/29325801/README.md File README.md (right): https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode213 README.md:213: {{string_name[The string description] String contents}} You need to point out explicitly that neither the string name nor description have any effect on the generated markup - they are merely context information visible to translators. https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode223 README.md:223: custom Jinja2 filters and global functions as documented below._ This section belongs under "Markdown format" - that's how Markdown files are being localized. You can add a note under "Raw html" saying that the localization approach there is identical to Markdown (it sort of says so already). https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode239 README.md:239: to the page `foopage`, the second will become a link to `http://example.com/`. This section is outdated and needs to be merged with the text above. https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode274 README.md:274: description can be specified as well. First parameter isn't string any more, it's default - you should point out what it means. https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode284 README.md:284: Unless a locale is specified the locale file matching the page name is used. I think you meant: "Unless a page is specified the locale file matching the name of the current page is used." https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode292 README.md:292: 'pagedata', 'translation_ratio', 'config', 'page'] Please don't list everything returned. In particular, things meant for internal use and the ones identical to the current page are irrelevant. It's really: head, body, available_locales, translation_ratio.
Patch Set 2 : Addressed Wladimir's feedback https://codereview.adblockplus.org/29325800/diff/29325801/README.md File README.md (right): https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode213 README.md:213: {{string_name[The string description] String contents}} On 2015/09/03 12:15:26, Wladimir Palant wrote: > You need to point out explicitly that neither the string name nor description > have any effect on the generated markup - they are merely context information > visible to translators. Done. https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode223 README.md:223: custom Jinja2 filters and global functions as documented below._ On 2015/09/03 12:15:26, Wladimir Palant wrote: > This section belongs under "Markdown format" - that's how Markdown files are > being localized. You can add a note under "Raw html" saying that the > localization approach there is identical to Markdown (it sort of says so > already). Done. https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode239 README.md:239: to the page `foopage`, the second will become a link to `http://example.com/`. On 2015/09/03 12:15:26, Wladimir Palant wrote: > This section is outdated and needs to be merged with the text above. Done. https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode274 README.md:274: description can be specified as well. On 2015/09/03 12:15:26, Wladimir Palant wrote: > First parameter isn't string any more, it's default - you should point out what > it means. Done. https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode284 README.md:284: Unless a locale is specified the locale file matching the page name is used. On 2015/09/03 12:15:26, Wladimir Palant wrote: > I think you meant: "Unless a page is specified the locale file matching the name > of the current page is used." Done. https://codereview.adblockplus.org/29325800/diff/29325801/README.md#newcode292 README.md:292: 'pagedata', 'translation_ratio', 'config', 'page'] On 2015/09/03 12:15:26, Wladimir Palant wrote: > Please don't list everything returned. In particular, things meant for internal > use and the ones identical to the current page are irrelevant. It's really: > head, body, available_locales, translation_ratio. Done.
https://codereview.adblockplus.org/29325800/diff/29325827/README.md File README.md (right): https://codereview.adblockplus.org/29325800/diff/29325827/README.md#newcode269 README.md:269: description can be specified as well. What's the difference between comment and description? As above, it should be explicitly noted that a comment is only something for translators. And maybe you should just call it "comment" everywhere (also in the Markdown section above)? https://codereview.adblockplus.org/29325800/diff/29325827/README.md#newcode287 README.md:287: ``` Why have this as a Python list? "Provided keys include: `head`, `body`, `available_locales`, `translation_ratio`."
Patch Set 3 : Addressed further feedback https://codereview.adblockplus.org/29325800/diff/29325827/README.md File README.md (right): https://codereview.adblockplus.org/29325800/diff/29325827/README.md#newcode269 README.md:269: description can be specified as well. On 2015/09/03 13:37:37, Wladimir Palant wrote: > What's the difference between comment and description? As above, it should be > explicitly noted that a comment is only something for translators. And maybe you > should just call it "comment" everywhere (also in the Markdown section above)? Well we seem to use both "comment" and "description" in different places. For example the JSON structure (that we're stuck with) uses "description" and for the parameter here we use "comment". I thought it would be nice to clarify here that they're one in the same. I've adjusted the wording again, what do you think? https://codereview.adblockplus.org/29325800/diff/29325827/README.md#newcode287 README.md:287: ``` On 2015/09/03 13:37:37, Wladimir Palant wrote: > Why have this as a Python list? "Provided keys include: `head`, `body`, > `available_locales`, `translation_ratio`." Done.
LGTM 
|
