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

Improve copy of console command examples #5397

Merged
merged 7 commits into from
Jul 31, 2024

Conversation

eth3lbert
Copy link
Contributor

@eth3lbert eth3lbert commented Jul 24, 2024

Summary

This PR improves the copy of the console command example by:

  • Preventing the selection of generic prompts and generic output
  • Lazily setting copy content by leveraging intersection observer

Most of the changes are inspired by opensafely/documentation#1461

Some other useful refs:

Resolves #5355

Test Plan

Copy link
Contributor Author

@eth3lbert eth3lbert left a comment

Choose a reason for hiding this comment

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

Noted: The current implementation is not restricted to console code blocks at this time.
If we find it too general, we could restrict it to specific languages by following the warning guidelines in https://mkdocstrings.github.io/recipes/#prevent-selection-of-prompts-and-output-in-python-code-blocks.

@zanieb zanieb self-assigned this Jul 24, 2024
@zanieb
Copy link
Member

zanieb commented Jul 24, 2024

Thanks! I'll play with this a bit.

@zanieb
Copy link
Member

zanieb commented Jul 24, 2024

Huh this isn't working for me on Vivaldi, it's working for you?

@eth3lbert
Copy link
Contributor Author

Huh this isn't working for me on Vivaldi, it's working for you?

Oh, really? I've tested it on Firefox and Arc without any issues.
I'll get Vivaldi and see what's going on.

@zanieb
Copy link
Member

zanieb commented Jul 24, 2024

Vivaldi is Chromium based fwiw. I can try Firefox too.

@zanieb
Copy link
Member

zanieb commented Jul 24, 2024

Doesn't work on Firefox for me either, let me see if it's something in my setup.

@zanieb
Copy link
Member

zanieb commented Jul 24, 2024

It's not my ad-blocker (uBlock Origin)

@eth3lbert
Copy link
Contributor Author

Vivaldi is Chromium based fwiw. I can try Firefox too.

Yes, I can reproduce the issue as well and have found a solution.

Instead of using

document.addEventListener("DOMContentLoaded", () => {
  setCopyText();
});

we should use the following instead

document$.subscribe(function() {
  setCopyText();
})

Follow the How to integrate with third-party JavaScript libraries guideline: https://squidfunk.github.io/mkdocs-material/customization/?h=javascript#additional-javascript

@eth3lbert
Copy link
Contributor Author

@zanieb A friendly ping, in case you missed the fix.

@zanieb
Copy link
Member

zanieb commented Jul 30, 2024

I was on vacation :) I'll take a look at it this week though!

@zanieb zanieb added documentation Improvements or additions to documentation preview Experimental behavior labels Jul 30, 2024
@zanieb
Copy link
Member

zanieb commented Jul 30, 2024

This still doesn't work for me in Vivaldi or Firefox on macOS :/

@eth3lbert
Copy link
Contributor Author

This still doesn't work for me in Vivaldi or Firefox on macOS :/

Weird, I just tested it in Vivaldi (6.8.3381.48), Firefox (128.0.3) and Arc (1.52.0) on macOS without any issue.

Have you seen any js errors in the developer tools?

@zanieb
Copy link
Member

zanieb commented Jul 30, 2024

Weird.. no errors in the developer tools console, just the "Enabled live reload" message. I can see extra.js was loaded in the network tab.

@zanieb
Copy link
Member

zanieb commented Jul 30, 2024

(Sorry I don't have the extra time to dig into what's going on right now)

@eth3lbert
Copy link
Contributor Author

No worries, we can revisit this later anytime.

@eth3lbert
Copy link
Contributor Author

The following is a non-lazy patch that has been modified to allow work with instant mode, which you might also like to try:

function getTextWithoutPromptAndOutput(targetSelector) {
  const targetElement = document.querySelector(targetSelector);

  // exclude "Generic Prompt" and "Generic Output" spans from copy
  const excludedClasses = ["gp", "go"];

  const clipboardText = [];
  [...targetElement.childNodes].map((node) => {
    // If the element does not contain the matching class,
    // add to the clipboard text array
    if (
      !excludedClasses.some((className) => node?.classList?.contains(className))
    ) {
      return clipboardText.push(node.textContent);
    }

    return null;
  });

  return clipboardText.join("").trim();
}

function patchCopyCodeButtons() {
  // select all "copy" buttons whose target selector is a <code> element
  [
    ...document.querySelectorAll(
      `button.md-clipboard[data-clipboard-target$="code"]`,
    ),
  ].map((btn) =>
    btn.setAttribute(
      "data-clipboard-text",
      getTextWithoutPromptAndOutput(btn.dataset.clipboardTarget),
    ),
  );
}

document$.subscribe(function () {
  patchCopyCodeButtons();
});

Although both lazy and non-lazy work for me.

@zanieb
Copy link
Member

zanieb commented Jul 31, 2024

Hey so uhh have we been talking about different things the whole time?

I'm pressing the "copy text" button (which includes the "$" in the clipboard).

I just realized if I try to highlight and copy the text it works as described here ("$" is excluded) :)

@eth3lbert
Copy link
Contributor Author

Hmmm, the results are not as expected.
The expected behavior would be to enable both regardless of whether

  1. the user selects and copies the text. (css part)
  2. clicks the copy button. (js part)

@zanieb
Copy link
Member

zanieb commented Jul 31, 2024

Ah tragic. I don't understand why the JS isn't working for me... I'll see if someone else on our side can reproduce.

@eth3lbert
Copy link
Contributor Author

Oh, and I think there's one more thing we could check first. Check the data attribute data-clipboard-text of the copy button. It's supposed to have the text without the $.

@zanieb
Copy link
Member

zanieb commented Jul 31, 2024

I don't see that attribute. I feel like I'm doing something wrong 😭

<button class="md-code__button" title="Copy to clipboard" data-clipboard-target="#__code_0 > code" data-md-type="copy"></button>

If I add

diff --git a/docs/js/extra.js b/docs/js/extra.js
index a8058688..8b08034c 100644
--- a/docs/js/extra.js
+++ b/docs/js/extra.js
@@ -25,6 +25,7 @@ function setCopyText() {
   const elements = document.querySelectorAll(
     'button.md-clipboard[data-clipboard-target$="code"]',
   );
+  console.log(elements);
   const observer = new IntersectionObserver((entries) => {
     entries.forEach((entry) => {
       // target in the viewport that have not been patched

I see NodeList [] in the console.

@zanieb
Copy link
Member

zanieb commented Jul 31, 2024

Doing something sloppy like this makes it work

diff --git a/docs/js/extra.js b/docs/js/extra.js
index a8058688..dd49ab48 100644
--- a/docs/js/extra.js
+++ b/docs/js/extra.js
@@ -23,8 +23,9 @@ function setCopyText() {
   const attr = "clipboardText";
   // all "copy" buttons whose target selector is a <code> element
   const elements = document.querySelectorAll(
-    'button.md-clipboard[data-clipboard-target$="code"]',
+    "#__code_0 > nav > button"
   );
+  console.log(elements);
   const observer = new IntersectionObserver((entries) => {
     entries.forEach((entry) => {
       // target in the viewport that have not been patched

@zanieb
Copy link
Member

zanieb commented Jul 31, 2024

(Is it possible it's a difference from the insiders build?)

@eth3lbert
Copy link
Contributor Author

Indeed, I can reproduce the issue on https://docs.astral.sh/uv/. There is also no button with the md-clipboard class. I'll change the class name.

@zanieb
Copy link
Member

zanieb commented Jul 31, 2024

Victory!

@zanieb
Copy link
Member

zanieb commented Jul 31, 2024

Thanks :)

docs/js/extra.js Outdated Show resolved Hide resolved
@eth3lbert
Copy link
Contributor Author

Thanks for your time and the review :)

@zanieb
Copy link
Member

zanieb commented Jul 31, 2024

Is there a compelling argument for the more complicated lazy-version or should we do the second patch you suggested?

@eth3lbert
Copy link
Contributor Author

The lazy version only patches elements within the viewport via IntersectionObserver, while the non-lazy version patches all elements. Therefore, for long pages or pages with a large number of copy buttons and content, the lazy version would be preferable.

The only drawback of the lazy version is browser compatibility.

You can find more information on browser compatibility here: https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver/IntersectionObserver#browser_compatibility

Currently, IntersectionObserver enjoys baseline wide availability. If you have concerns and need to support even older browsers, then I would suggest using the non-lazy version.

Co-authored-by: eth3lbert <eth3lbert+dev@gmail.com>
@eth3lbert
Copy link
Contributor Author

@zanieb Could you also indent the suggestion before merging :)

docs/js/extra.js Outdated Show resolved Hide resolved
docs/js/extra.js Outdated Show resolved Hide resolved
@zanieb
Copy link
Member

zanieb commented Jul 31, 2024

Thanks !

@zanieb zanieb enabled auto-merge (squash) July 31, 2024 14:50
@zanieb zanieb merged commit 8994768 into astral-sh:main Jul 31, 2024
46 checks passed
@eth3lbert eth3lbert deleted the doc-copy-console branch July 31, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation preview Experimental behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve copy of console command examples
2 participants