Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1452373002:
Add a table of contents to the c++11 style guide. (Closed)
DescriptionAdd a table of contents to the c++11 style guide.
It's hard/annoying to find the library allowed features by scrolling,
now we can link to them all, yay.
R=thakis@chromium.org
BUG=554287
Committed: https://crrev.com/4fd49fe4e34960db9412ff883e5659f0307adcf8
Cr-Commit-Position: refs/heads/master@{#360409}
Patch Set 1 #
Total comments: 2
Patch Set 2 : toc: . #
Total comments: 6
Messages
Total messages: 16 (5 generated)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1452373002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1452373002/1
https://codereview.chromium.org/1452373002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1452373002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:59: <h2 id="whitelist"><a name="core-whitelist"/>C++11 Allowed Features</h2> Isn't it <a></a> due to doctype html? (http://stackoverflow.com/questions/3558119/are-self-closing-tags-valid-in-html5)
Description was changed from ========== Add a table of contents to the c++11 style guide. It's hard/annoying to find the library allowed features by scrolling, now we can link to them all, yay. R=thakis@chromium.org ========== to ========== Add a table of contents to the c++11 style guide. It's hard/annoying to find the library allowed features by scrolling, now we can link to them all, yay. R=thakis@chromium.org BUG=554287 ==========
PTAL https://codereview.chromium.org/1452373002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1452373002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:59: <h2 id="whitelist"><a name="core-whitelist"/>C++11 Allowed Features</h2> On 2015/11/17 23:06:23, Nico wrote: > Isn't it <a></a> due to doctype html? > (http://stackoverflow.com/questions/3558119/are-self-closing-tags-valid-in-html5) TIL.
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1452373002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1452373002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hooray, an html review :-) lgtm with the </ol> comments fixed, and an optional nit (that applies to 6 places) too https://codereview.chromium.org/1452373002/diff/20001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1452373002/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:48: </ol></li> all your </li>s have an </ol> too for some reason – I think this </ol> here needs to go… https://codereview.chromium.org/1452373002/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:52: </ol></li> …and this… https://codereview.chromium.org/1452373002/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:56: </ol></li> …and this… https://codereview.chromium.org/1452373002/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:57: </ol> …but this one should stay https://codereview.chromium.org/1452373002/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:324: <h3 id="blacklist_banned"><a name="core-blacklist"></a>C++11 Banned Features</h3> (nit: since you have an ending tag now, you might as well put the text "C++11 banned features" inside the <a> so that the anchor semantically refers to it and not the empty white space in front of it)
https://codereview.chromium.org/1452373002/diff/20001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1452373002/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:48: </ol></li> On 2015/11/18 01:44:16, Nico (vacation Nov 18-19) wrote: > all your </li>s have an </ol> too for some reason – I think this </ol> here > needs to go… I have a nested OL inside the LI. The opening is on line 45.
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1452373002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1452373002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4fd49fe4e34960db9412ff883e5659f0307adcf8 Cr-Commit-Position: refs/heads/master@{#360409} |
