Re: RFR: JDK-8325874: Improve checkbox-based interface in summary pages [v2]
> Please review a change to improve the user experience and implementation for > the checkbox-based interface to selectively display content on API summary > pages: > > - Add a checkbox to toggle (select/unselect) all checkboxes in the page. > - Remove the `Table.setAlwaysShowDefaultTab(boolean)` setter as it is > redundant and can be replaced by `!renderTabs`. > - Consolidate the code to generate checkboxes into a new protected > `SummaryListWriter.getCheckbox` method. Hannes Wallnöfer has updated the pull request incrementally with one additional commit since the last revision: Review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/18194/files - new: https://git.openjdk.org/jdk/pull/18194/files/cb37764d..d7460e8d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18194=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18194=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18194.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18194/head:pull/18194 PR: https://git.openjdk.org/jdk/pull/18194
Re: RFR: JDK-8325874: Improve checkbox-based interface in summary pages
On Wed, 13 Mar 2024 14:30:01 GMT, Hannes Wallnöfer wrote: > > if you check "all" and then uncheck something else, then "all" will remain > > selected. That is a confusing state. > > I agree, but I think the behavior of the checkbox is as it should be, and > it's the label that is slightly misleading. The intended behavior of the > checkbox is set all other checkboxes to a unified state which can be either > "on" or "off", not to reflect the state of other checkboxes. > > So maybe the correct label would be "toggle all" rather than just "all". > Admittedly I have used the shorter "all" label to limit the length of the > already long line of checkboxes. Do you think using "toggle all" as label > would make things better? No, I don't think that "toggle all" would be better. It's the checkbox that slightly bothers me, not that checkbox's label. One minimalistic, consistent approach that comes to mind is a stateless button that inverts selection. But admittedly, your suggestion is already implemented and is _less nerdy_. Don't bother changing it; it is a nit. - PR Comment: https://git.openjdk.org/jdk/pull/18194#issuecomment-1994584561
Re: RFR: JDK-8325874: Improve checkbox-based interface in summary pages
On Mon, 11 Mar 2024 18:09:41 GMT, Pavel Rappo wrote: > if you check "all" and then uncheck something else, then "all" will remain > selected. That is a confusing state. I agree, but I think the behavior of the checkbox is as it should be, and it's the label that is slightly misleading. The intended behavior of the checkbox is set all other checkboxes to a unified state which can be either "on" or "off", not to reflect the state of other checkboxes. So maybe the correct label would be "toggle all" rather than just "all". Admittedly I have used the shorter "all" label to limit the length of the already long line of checkboxes. Do you think using "toggle all" as label would make things better? - PR Comment: https://git.openjdk.org/jdk/pull/18194#issuecomment-1994536241
Re: RFR: JDK-8325874: Improve checkbox-based interface in summary pages
On Mon, 11 Mar 2024 13:37:54 GMT, Hannes Wallnöfer wrote:
> Please review a change to improve the user experience and implementation for
> the checkbox-based interface to selectively display content on API summary
> pages:
>
> - Add a checkbox to toggle (select/unselect) all checkboxes in the page.
> - Remove the `Table.setAlwaysShowDefaultTab(boolean)` setter as it is
> redundant and can be replaced by `!renderTabs`.
> - Consolidate the code to generate checkboxes into a new protected
> `SummaryListWriter.getCheckbox` method.
Looks okay.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/NewAPIListWriter.java
line 86:
> 84: Content tabs = HtmlTree.DIV(HtmlStyle.checkboxes,
> 85: contents.getContent("doclet.New_API_Checkbox_Label"));
> 86: // Table column ids are 1-based
On line 82, which I cannot add a comment to on GitHub, there's this:
List releases = configuration.newAPIPageBuilder.releases;
It might be better to change it to this:
List releases = builder.releases;
-
Marked as reviewed by prappo (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18194#pullrequestreview-1934169258
PR Review Comment: https://git.openjdk.org/jdk/pull/18194#discussion_r1523251733
Re: RFR: JDK-8325874: Improve checkbox-based interface in summary pages
On Mon, 11 Mar 2024 13:37:54 GMT, Hannes Wallnöfer wrote: > Please review a change to improve the user experience and implementation for > the checkbox-based interface to selectively display content on API summary > pages: > > - Add a checkbox to toggle (select/unselect) all checkboxes in the page. > - Remove the `Table.setAlwaysShowDefaultTab(boolean)` setter as it is > redundant and can be replaced by `!renderTabs`. > - Consolidate the code to generate checkboxes into a new protected > `SummaryListWriter.getCheckbox` method. It'd work for a few more years, but it doesn't scale well. On the other hand, it is very simple: https://github.com/openjdk/jdk/assets/32523691/26919228-db98-4041-833f-f01c34125331;> Nit: if you check "all" and then uncheck something else, then "all" will remain selected. That is a confusing state. https://github.com/openjdk/jdk/assets/32523691/49c1d803-57b3-483b-857f-64781bf96ce0;> I'll have a look at the code change a bit later. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/18194#issuecomment-1989120925
RFR: JDK-8325874: Improve checkbox-based interface in summary pages
Please review a change to improve the user experience and implementation for the checkbox-based interface to selectively display content on API summary pages: - Add a checkbox to toggle (select/unselect) all checkboxes in the page. - Remove the `Table.setAlwaysShowDefaultTab(boolean)` setter as it is redundant and can be replaced by `!renderTabs`. - Consolidate the code to generate checkboxes into a new protected `SummaryListWriter.getCheckbox` method. - Commit messages: - Add bug id to tests - JDK-8325874: Improve checkbox-based interface in summary pages Changes: https://git.openjdk.org/jdk/pull/18194/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18194=00 Issue: https://bugs.openjdk.org/browse/JDK-8325874 Stats: 140 lines in 9 files changed: 55 ins; 52 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/18194.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18194/head:pull/18194 PR: https://git.openjdk.org/jdk/pull/18194
