On Thu, 30 Jun 2022 15:14:40 GMT, Hannes Wallnöfer <hann...@openjdk.org> wrote:
> Please review an enhancement to the preview page to add a list of preview > features and allow users to explore the preview APIs by feature. > > While the changes for the enhancement itself are not overly complex, the work > entailed a moderate cleanup of other summary API lists (Deprecated and New > API pages) as well as a few unrelated summary pages that happened to share > some of the CSS classes and HTML structure. > > The change itself starts with a new internal > `jdk.internal.javac.PreviewFeature.JEP` annotation interface to add JEP > information to the nearby `jdk.internal.javac.PreviewFeature.Feature` enum. > This JEP information is retrieved by new methods in the javadoc `Utils` class > and then fetched by `PreviewAPIListBuilder` which passes it on to > `PreviewListWriter`. > > The superclass of `PreviewListWriter` and other API summary writer classes, > `SummaryListWriter`, gets a new `addContentSelectors(Content)` method to add > the checkboxes to show/hide various parts of the page content. The class is > also now abstract as this and other methods do not have useful > implementations in the base class. > > Another change to `SummaryListWriter` is that `pageMode`, `description`, > `headContent` and `titleKey` are no longer fields in the class but rather > sent to the `generateSummaryListFile` method as parameters since they are > mostly just used locally in that method. On the other hand, the associated > Builder is used in many places and previously had to be retrieved from the > configuration object or passed around as parameter, so that is now set as a > final field in `SummaryListWriter`. > > Finally, a few words about changes in CSS and HTML: The "Contents" list > containing various kinds of elements in the API list was previously contained > in the `<div class="header">` element that also contains the page header, and > the stylesheet used the `.header ul` selector to style the list. Now that > the checkboxes separate the page header from the contents list it felt wrong > to put all this in the header div (most other pages only use the div for the > header itself). I also didn't like the indirect CSS selector too much, > `.header ul` is not very indicative of what it applies to. > > I therefore decided to create a new dedicated CSS class for the contents list > called `contents-list`, and move the `<ul>` element out of the header div. > Apart from the summary API pages, this change also affects the "Constant > Values" page as well as the top and package level "Tree" (class hierarchy) > pages. I made sure the contents list in these pages look the same as before. > > The list items in the contents list now each have an `id` attribute to allow > hiding the list item if no content for the element kind is currently > selected/visible. The value of the `id` attribute uses the format > `contents-<element-kind>`. > > There is one additional CSS class called `preview-feature-list` used for the > list of preview feature JEPs. > > Demo output for the new preview page as well as other pages is available here: > http://cr.openjdk.java.net/~hannesw/8287597/api.03/preview-list.html Mostly excellent. There are some minor comments inline. There are more "English phrases" (recognized as "strings with words separated by spaces") than I expected, at least some of which will end up in generated docs. But that being said, I guess this is a JDK-only feature, and we don't translate the source going into our API docs. src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 91: > 89: /** JEP number */ > 90: int number() default 0; > 91: /** JEP title */ suggest noting the format (i.e. plain text, not HTML, not a resource key) src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SummaryListWriter.java line 164: > 162: protected void addIndexLink(HtmlId id, String headingKey, Content > content) { > 163: var li = HtmlTree.LI(links.createLink(id, > 164: > contents.getContent(headingKey))).setId(HtmlId.of("contents-" + id.name())); Suggest adding a comment that `"contents-" + id` appears in the JavaScript code as well. You presumably can't change one without the other (and can't use a common function across the language barrier.) src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties line 125: > 123: doclet.Preview_API=Preview API > 124: doclet.Preview_API_Checkbox_Label=Show preview API for: > 125: doclet.Preview_JEP_URL=https://openjdk.java.net/jeps/{0} Use `openjdk.org` src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/stylesheet.css line 243: > 241: } > 242: ul.contents-list li { > 243: font-size: 13px; Do these values have to be normalized into `.em` units, to align with new themes? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 1346: > 1344: * @return the map of PreviewFeature.JEP annotation element values, > or an empty map > 1345: */ > 1346: public Map<? extends ExecutableElement, ? extends AnnotationValue> > getJepInfo(String feature) { This method should borderline be in `Workarounds.java` because you are accessing javac internal features, even if only reflectively. ------------- PR: https://git.openjdk.org/jdk/pull/9336
