On Tue, 9 Sep 2025 09:19:41 GMT, Nizar Benalla <[email protected]> wrote:
>> Please review this patch to add a toggle to order the member details in the >> table of contents in lexical order. The selected choice is stored and >> preserved. >> >> Here is a preview of the new toggle. >> >> >> https://github.com/user-attachments/assets/55c81e4b-5fc0-416e-8946-53aede419640 > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > member details should also be sorted This looks very good. I have added a few minor comments and questions inline. One issue I noticed is that it also sorts section headings in the main description of classes in the TOC, which it shouldn't do. One class where you can see this is `java.lang.Thread`. So I guess sorting needs to be limited to details sections. I still need a bit more time to review the JS sorting code, but I thought I shouldn't let you wait for the other feedback. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TableOfContents.java line 118: > 116: header.add(Entity.NO_BREAK_SPACE) > 117: .add(HtmlTree.BUTTON(HtmlStyles.tocSortToggle) > 118: .put(HtmlAttr.ID, HtmlIds.TOC_ORDER_TOGGLE) Should use `setId(HtmlId)` src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template line 26: > 24: > 25: const sortLexicalLabel = "Sort lexicographically"; > 26: const sortSourceLabel = "Sort by source order"; These should be localized messages as the ones in the preceding lines. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template line 653: > 651: function textForLi(li) { > 652: var a = li.querySelector(":scope > a"); > 653: return (a ? a.textContent : li.textContent).trim(); Why is this needed? Isn't there always a link? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template line 766: > 764: var img = btn.querySelector("img"); > 765: if (img) img.alt = next; > 766: }); It would be nice to have a visual clue for the state of the button. For example, if lexicographic order is applied, the button could have a darker (or lighter) background color. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/toggle.svg line 1: > 1: <?xml version="1.0" encoding="UTF-8"?> I think the icon works well, but the black is a bit striking. It should use a dark gray color for the light theme (via stroke attribute). For example, the copy-to-clipboard and link icons use `#505050` and `#404040`. This should result in a more off-white for the dark theme. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/DocPaths.java line 119: > 117: > 118: /** The name of the table of contents toggle icon file. */ > 119: public static final DocPath TOGGLE_SVG = > DocPath.create("toggle.svg"); I think "toggle" is not a good description for the icon. Something "sort" or "a-z" would describe it better. ------------- PR Review: https://git.openjdk.org/jdk/pull/26322#pullrequestreview-3210375366 PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2339893967 PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2339984402 PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2340083835 PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2340023404 PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2340036014 PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2339948382
