GNOME Bugzilla – Bug 726736
Review of gnome-robots 3.11.90 documentation
Last modified: 2015-01-25 22:27:44 UTC
Patches created during a review of gnome-robots in GNOME 3.11 (gnome-robots version 3.11.90). Patch for new images
Created attachment 272415 [details] [review] Patch with new images
Created attachment 272416 [details] [review] Updated makefile.am
Created attachment 272417 [details] [review] Update to appearance.page with new images
Created attachment 272418 [details] [review] Update to basics.page with starting new game from toolbar
Created attachment 272420 [details] [review] Update to score.page with new themes and high score info
Created attachment 272421 [details] [review] Update to game-types.page: reordering and reformatting
Created attachment 272422 [details] [review] Update to controls-default.page for keyboard navigation and alt text
Comment on attachment 272415 [details] [review] Patch with new images These files either need to be used and added to the Makefile.am in the same patch or to not be added.
Review of attachment 272416 [details] [review]: Merge this into the patch that adds these.
Review of attachment 272417 [details] [review]: ::: help/C/appearance.page @@ +1,2 @@ <page xmlns="http://projectmallard.org/1.0/" + xmlns:its="http://www.w3.org/2005/11/its" If you're going to be adding the its namespace to pages, you may as well use it somewhere :) @@ +74,3 @@ <p>ufo</p> </item> </list> I would actually remove this list as it doesn't offer anything interesting over what the UI does and adds a maintenance burden, as well as a translation burden.
Review of attachment 272418 [details] [review]: ::: help/C/basics.page @@ +18,3 @@ + <credit type="editor"> + <name>Rachel Dunstan</name> + <email>dunstanrc@gmail.com</email> You can, and probably should use the ITS namespace to make your email address untranslatable. @@ +32,3 @@ + <gui style="menuitem">New</gui></guiseq>, click the + <media type="image" its:translate="no" src="figures/new.png">New toolbar + button</media> button on the toolbar, or press Don't break the line between the <gui> tags. The content of the <media> tag needs to be translatable. You can use a <span>.
Review of attachment 272420 [details] [review]: ::: help/C/scores.page @@ +30,3 @@ + points when they die.</p> + +<section id="high-low-risk"> Did you check whether anything linked to the old section ID? @@ +31,3 @@ + +<section id="high-low-risk"> +<title>High and low risk enemies by theme</title> Keep original indentation. @@ +37,3 @@ + <table frame="top bottom" rules="rows"> + <thead> + <tr><td><p>Game theme</p></td> <td><p>High-risk enemy (20 points)</p></td> Please break the line between each <td> as that's what you're doing lower down. Give <tr> its own line as well. @@ +43,3 @@ + <tr><td><p>anthills</p></td> + <td><p> + <media type="image" its:translate="no" There should be no space between <p> and <media> unless you actually want one there. @@ +45,3 @@ + <media type="image" its:translate="no" + src="figures/anthills-high-risk.png" height="20" width="20">High-risk + anthill</media></p></td> Alt texts should be translatable. @@ +149,3 @@ + +<p>Select <guiseq><gui style="menu">Game</gui> + <gui style="menuitem">Scores</gui></guiseq> to view the high scores.</p> Indentation. Don't break the line between <gui> tags. Best indentation in this case would look like: <p>Select <guiseq><gui style="menu">Game</gui><gui style="menuitem">Scores</gui></guiseq> to view the high scores.</p> @@ +151,3 @@ + <gui style="menuitem">Scores</gui></guiseq> to view the high scores.</p> + + <p> Scores are recorded separately for each combination of Extra space at the start.
Review of attachment 272421 [details] [review]: ::: help/C/game-types.page @@ +44,3 @@ <td><p>Game Type</p></td> <td><p>Initial safe <link xref="moves#teleport">teleports</link></p> + </td> This should have a fair few spaces more than it does. Check the rest of the page for indentation too. Also, you should close this tag on the same line as </p>
Review of attachment 272422 [details] [review]: ::: help/C/controls-default.page @@ +76,3 @@ <td><p>Stay in the same position</p></td> + <td><p><key>Begin</key> (marked only as <key>5</key> on some keypads) + </p></td> Close the tags on the same line as the last word in the paragraph. So either: …some keypads)</p></td> or: …some keypads)</p></td> @@ +120,3 @@ <title>Wait for the robots</title> <p>Click the + <media type="image" its:translate="no" src="figures/wait.png">Wait toolbar button</media> Alt text needs to be translatable.
Created attachment 272589 [details] [review] Merged patches for new images and makefile
Review of attachment 272589 [details] [review]: It makes logical sense to merge this with the patch where you update the links to the images, what do you think?
Review of attachment 272589 [details] [review]: Opps, wrong status!
Created attachment 272689 [details] [review] Merger of patches for new files and scores.page (where files used)
Created attachment 272690 [details] [review] Removed list of themes in appearance.page
Created attachment 272691 [details] [review] Update to basics.page, with ITS tags
Created attachment 272692 [details] [review] Updated games-types.page, with corrected section links
Created attachment 272693 [details] [review] Updated controls-default.page, with alt text
Review of attachment 272689 [details] [review]: There is some whitespace in the patch: remove it. The easiest way to check is to set color.ui to auto in your global git configuration and to look at the commit (for example, using git show). You also broke a link here: find it and fix it in this patch as well. Use yelp-check which comes as part of yelp-tools. ::: help/C/scores.page @@ +33,3 @@ + <title>High and low risk enemies by theme</title> + + <p>The enemies in a game depend on the <link xref="appearance">game theme</link> chosen.</p> Reflow. ::: help/Makefile.am @@ +5,3 @@ + figures/anthills-high-risk.png \ + figures/anthills-low-risk.png \ + figures/boo-high-risk.png \ Don't change the style here: end each line with a ' \' instead of varying number of spaces.
Review of attachment 272690 [details] [review]: Pushed to master in commit 83934665f87e36a4671bded5b55ef13812d80971 Please make sure to mention the bug in the patch subject as 'bug 726736' or link to it in the body of the commit message.
Kittykat, I can't find the broken link: if I run yelp-check links on *.page in my working directory, and also yelp-check hrefs, neither reports anything. I've also opened everything in yelp, and checked it, and can't find anything. Help? I'll make sure I add bug references, sorry.
Review of attachment 272691 [details] [review]: There's whitespace in this patch, please remove it.
Review of attachment 272692 [details] [review]: Fix whitespace errors.
Review of attachment 272693 [details] [review]: Fix whitespace errors.
(In reply to comment #25) > Kittykat, I can't find the broken link: if I run yelp-check links on *.page in > my working directory, and also yelp-check hrefs, neither reports anything. I've > also opened everything in yelp, and checked it, and can't find anything. Help? It's possible that you fix it in a later patch. I'm assuming that you are working locally on master instead of one branch per patch? Then the easiest way to test your patches is either to do an interactive rebase and edit that commit, or reset your working tree to master, then apply that one patch and check again. You'll also want to do a 'git pull --rebase' as I pushed one of your patches to master.
Created attachment 272993 [details] [review] Merger of patches for new files and scores.page. Whitespace fixed Link broken in this patch is fixed in patch for game-types.page. (New section ID added here, linked in games-types.page, which had links to old section IDs).
Created attachment 272994 [details] [review] Updated basics.page with ITS tags. Whitespace fixed
Created attachment 272995 [details] [review] Updated controls-default.page, with alt text. Whitespace fixed
Created attachment 272996 [details] [review] Updated games-types.page, with corrected section links. Whitespace corrected Also fixes broken link introduced in patch for scores.page (due to update of section IDs).
Review of attachment 272994 [details] [review]: ::: help/C/basics.page @@ +31,3 @@ + <p>Select <guiseq><gui style="menu">Game</gui><gui + style="menuitem">New</gui></guiseq>, click the + <media type="image" its:translate="no" src="figures/new.png"> This figure does not appear to be in figures/ nor in the Makefile.am
Review of attachment 272995 [details] [review]: The images and updated to the Makefile.am appear to be missing here too
The images are in the patch for the makefile, the figures and the scores https://bug726736.bugzilla-attachments.gnome.org/attachment.cgi?id=272993. Should I merge that patch, the controls-default.page patch and the basics.page patch? I haven't so far, because of the "one patch per file" rule you originally mentioned.
(In reply to comment #38) > The images are in the patch for the makefile, the figures and the scores > https://bug726736.bugzilla-attachments.gnome.org/attachment.cgi?id=272993. > Should I merge that patch, the controls-default.page patch and the basics.page > patch? I haven't so far, because of the "one patch per file" rule you > originally mentioned. They are better kept as one logical change per patch, but each patch should be self contained in that if you use a new image, then that patch should also include that image and Makefile.am change. What threw me off was that the commit message implied that the images should have been in those two patches. Just to be sure, all of the yelp-check tools should show no errors if run after every patch is applied on top of master. Other things to watch for are changing IDs and checking links to them, adding new pages and deleting files or pages.
Comment on attachment 272994 [details] [review] Updated basics.page with ITS tags. Whitespace fixed Pushed to master in commit 0954e4f88b08ee6c7038641a992de1296fbc3a7b after adding the image and Makefile change to this patch.
Comment on attachment 272995 [details] [review] Updated controls-default.page, with alt text. Whitespace fixed Pushed to master in commit a0a23b0ab4cd47a95095ff3feafc7a99d0998d0f after some minor changes.
Review of attachment 272996 [details] [review]: After applying this patch and running 'yelp-check links help/C/*.page', I get the following errors: game-types: scores#high-low-risk game-types: scores#high-low-risk
Created attachment 273302 [details] [review] Merger of patches for new files and scores.page. Whitespace fixed. Updates section links in game-types.page
Created attachment 273305 [details] [review] Updated games-types.page. Whitespace corrected. Section links in game-types.page, previously updated in this patch, are now updated in the patch for new files, Makefile.am and scores.page.
Hi, did this get forgotten?
Somebody of docs needs to review the latest two patches.
The changes seem straightforward, so I've pushed them.