Skip to content

JDK-8277420: Provide a way to copy the hyperlink to a doc element to the clipboard #8817

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

Closed
wants to merge 5 commits into from

Conversation

hns
Copy link
Member

@hns hns commented May 20, 2022

This is a CSS/JavaScript only change to implement copy-to-clipboard functionality for all headers (h1 - h6) that have an id attribute associated with them. The following element-attribute patterns are supported (using <h2> as an example):

  • <section id="..."><h2> (generated by javadoc)
  • <h2 id="..."> (commonly used)
  • <h2><a id="..."> (legacy)

The change includes a consolidation of the CSS styles used to render copy-to-clipboard buttons, of which we have now three kinds: for snippets, for the link on the search page, and for headers. There is now a base CSS class called "copy" that defines the styles shared by all copy-to-clipboard buttons, and additional CSS properties for the concrete "subclasses".

API docs generated with this change can be viewed here (top level files and java.base module):
http://cr.openjdk.java.net/~hannesw/8277420/api.03/


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-8277420: Provide a way to copy the hyperlink to a doc element to the clipboard

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8817/head:pull/8817
$ git checkout pull/8817

Update a local copy of the PR:
$ git checkout pull/8817
$ git pull https://git.openjdk.java.net/jdk pull/8817/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8817

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8817.diff

@bridgekeeper
Copy link

bridgekeeper bot commented May 20, 2022

👋 Welcome back hannesw! 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 Pull request is ready for review label May 20, 2022
@openjdk
Copy link

openjdk bot commented May 20, 2022

@hns The following label will be automatically applied to this pull request:

  • javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the javadoc javadoc-dev@openjdk.org label May 20, 2022
@mlbridge
Copy link

mlbridge bot commented May 20, 2022

Webrevs

@JornVernee
Copy link
Member

Very nice!

I'm trying this out by getting a link to the "Signature Polymorphism" section in the class javadoc here: http://cr.openjdk.java.net/~hannesw/8277420/api.02/java.base/java/lang/invoke/MethodHandle.html

The source for that is <h2><a id="sigpoly"></a>Signature polymorphism</h2> but no link/copy button seems to be generated. Shouldn't this also have a button? (seems to fall in the legacy category)

@JornVernee
Copy link
Member

JornVernee commented May 20, 2022

It seems to work in other cases, like here: http://cr.openjdk.java.net/~hannesw/8277420/api.02/java.base/java/lang/foreign/Linker.html (e.g. the "Downcall method handles" section)

Where the text is contained within the <a> tag.


Also, I notice that the button only appears after hovering over the text of the heading in the case above, but not when just hovering over the place where the button would appear itself (and not touching the text). The hover area feels a bit small. Would it be possible to make the area bigger? For instance, the entire width of the page at the height of the header? For methods this already seems to be the case as well.

@hns
Copy link
Member Author

hns commented May 23, 2022

Thanks for the valuable feedback, @JornVernee! It turns out that both problems you observed (empty <a id=... not working at all and other "legacy" headers only showing the icon when hovering over the text) had the same cause, which was that the hover event handler was registered on the <a> element instead of the header.

I've added a fix for this, you can test it here:
http://cr.openjdk.java.net/~hannesw/8277420/api.03/java.base/java/lang/invoke/MethodHandle.html#sigpoly

Regarding the size of the icon/button, it's true that it's on the smaller side. The reason is that I tried to fit it into the line size of the header both for visual and technical reasons (easier to size using relative rather than absolute measures). If it was any bigger, the header line height would increase when hovering over it. If it turns out that more people find it too small we can increase the size with a bit more work (absolute positioning and sizing).

@JornVernee
Copy link
Member

JornVernee commented May 23, 2022

Thanks! The fixed version works fine for me as well.

Regarding the size of the icon/button, it's true that it's on the smaller side. The reason is that I tried to fit it into the line size of the header both for visual and technical reasons (easier to size using relative rather than absolute measures). If it was any bigger, the header line height would increase when hovering over it. If it turns out that more people find it too small we can increase the size with a bit more work (absolute positioning and sizing).

I don't think the icon is too small. I just meant that the area where you needed to hover the mouse to make it appear seemed small. But, with your latest version it seems good to me, now that the copy button will show up when hovering anywhere on the line of the header.

Copy link
Member

@pavelrappo pavelrappo left a comment

Choose a reason for hiding this comment

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

Code:

  • Your JavaScript code looks very readable to me, and I don't know JavaScript; nice.
  • Don't forget to update the copyright years.

UI:

Looks good.

@openjdk
Copy link

openjdk bot commented May 26, 2022

@hns 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:

8277420: Provide a way to copy the hyperlink to a doc element to the clipboard

Reviewed-by: prappo

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 Pull request is ready to be integrated label May 26, 2022
@pavelrappo
Copy link
Member

Ensure TestSnippetUnnamedPackage passes before integrating.

@hns
Copy link
Member Author

hns commented May 27, 2022

/integrate

@openjdk
Copy link

openjdk bot commented May 27, 2022

Going to push as commit 37ecbb4.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 27, 2022
@openjdk openjdk bot closed this May 27, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 27, 2022
@openjdk
Copy link

openjdk bot commented May 27, 2022

@hns Pushed as commit 37ecbb4.

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

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 javadoc javadoc-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

3 participants