Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8321573: Improve Platform.Preferences documentation #1301

Closed
wants to merge 3 commits into from

Conversation

mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Dec 9, 2023

This PR enhances the documentation of Platform.Preferences.accentColor:

   /**
-   * The accent color.
+   * The accent color, which can be used to highlight the active or important part of a
+   * control and make it stand out from the rest of the user interface. It is usually a
+   * vivid color that contrasts with the foreground and background colors.

Additional changes include alternating row colors for tables.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8321573: Improve Platform.Preferences documentation (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1301/head:pull/1301
$ git checkout pull/1301

Update a local copy of the PR:
$ git checkout pull/1301
$ git pull https://git.openjdk.org/jfx.git pull/1301/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1301

View PR using the GUI difftool:
$ git pr show -t 1301

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1301.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 9, 2023

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Dec 9, 2023
@mlbridge
Copy link

mlbridge bot commented Dec 9, 2023

Webrevs

Copy link
Collaborator

@nlisker nlisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed unrelated points:

  1. The properties use the phrasing "If the platform does not report..." except for the one for color scheme, which uses "The value of this property defaults to". Might want to use the same phrasing for color scheme too.
  2. The default value for the foreground color is specified as Color.BLACK instead of Color#BLACK.

Comment on lines 610 to 612
* The accent color, which is usually a vivid color that contrasts with the foreground
* and background colors. It can be used to highlight the active or important part of a
* control and make it stand out from the rest of the user interface.
Copy link
Collaborator

@nlisker nlisker Dec 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more important to say what it's used for than what value it should have, so I would switch the order:

The accent color, which can be used to highlight the active or important part of a
control and make it stand out from the rest of the user interface. It is usually a vivid
color that contrasts with the foreground and background colors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's better.

@mstr2 mstr2 changed the title 8321573: Enhance documentation for Platform.Preferences.accentColor 8321573: Improve Platform.Preferences documentation Dec 9, 2023
Copy link
Collaborator

@nlisker nlisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Let's keep it open until Monday to give others a chance to review.

@@ -585,7 +593,7 @@ public interface Preferences extends ObservableMap<String, Object> {
/**
* The color used for background regions.
* <p>
* If the platform does not report a background color, this property defaults to {@code Color.WHITE}.
* If the platform does not report a background color, this property defaults to {@link Color#WHITE}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I don't think you need a link here because it's linked in the default value tag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Only one one of them needs the link (and yes, it's minor).

@openjdk
Copy link

openjdk bot commented Dec 9, 2023

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8321573: Improve Platform.Preferences documentation

Reviewed-by: nlisker, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Dec 9, 2023
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated docs look good. I left a comment about the html id of the tables.

@@ -470,10 +470,9 @@ public static Preferences getPreferences() {
* always available.
* <p>
* The following preferences are potentially available on the specified platforms:
* <table id="preferences-table">
* <caption>List of platform preferences</caption>
* <table id="preferences-table" class="striped">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend changing the ID to id="preferences-table-windows" and adding id="preferences-table-macos" and id="preferences-table-linux" to the other two.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if you do that, you will need to change the link on line 448 of this file.

@@ -585,7 +593,7 @@ public interface Preferences extends ObservableMap<String, Object> {
/**
* The color used for background regions.
* <p>
* If the platform does not report a background color, this property defaults to {@code Color.WHITE}.
* If the platform does not report a background color, this property defaults to {@link Color#WHITE}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Only one one of them needs the link (and yes, it's minor).

@openjdk openjdk bot removed the ready Ready to be integrated label Dec 9, 2023
@openjdk openjdk bot added the ready Ready to be integrated label Dec 11, 2023
@mstr2
Copy link
Collaborator Author

mstr2 commented Dec 11, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Dec 11, 2023

Going to push as commit 60476ef.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 11, 2023
@openjdk openjdk bot closed this Dec 11, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Dec 11, 2023
@openjdk
Copy link

openjdk bot commented Dec 11, 2023

@mstr2 Pushed as commit 60476ef.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@mstr2 mstr2 deleted the fixes/accent-color-doc branch May 31, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants