Hide buttons generated by the `TryExamples` directive by default #173

agriyakhetarpal · 2024-05-01T16:46:47Z

Description

This PR hides the buttons from the TryExamples directive by default. This is helpful because the current method of ignoring particular pages based on their URL first loads the buttons and then hides them, which means that they are briefly displayed on that page in the documentation and then hidden – keeping them hidden and then selectively enabling them will minimise this latency and improve user experience.

Changes made

Made all try_examples_button buttons hidden at the time of generation by adding this keyword to the classList
Updated the loadTryExamplesConfig function in jupyterlite_sphinx.js to check for the try_examples_button hidden class instead and then remove hidden from the classList

Additional context

N/A

agriyakhetarpal · 2024-05-01T16:51:34Z

I have rebased this right now and opened a PR because I think it will be better for visibility, plus I would appreciate some help towards bringing this to completion. It's not fully working right now since the directives still seem to contain the try_examples_button hidden and therefore are not displayed, even when I try to remove the keyword from the classList. It has been a while since I worked on this, so I'm not sure what I was debugging back then.

Carreau

I see why this is reasonable, but isn't that going to have the opposite effect on page where the button should be visible, where the page will load and then jump around as buttons are added ?

.gitignore

jupyterlite_sphinx/jupyterlite_sphinx.js

agriyakhetarpal · 2024-05-02T13:20:57Z

I see why this is reasonable, but isn't that going to have the opposite effect on page where the button should be visible, where the page will load and then jump around as buttons are added ?

Thanks for the review, @Carreau. Yes, this will be a slight issue when someone reloads a page and if they are in the middle of a page when doing so – but at the top of the page, it should load as normal, because from there the buttons will be appended to the layout rather than being inserted into a position where the text moves because of the insertion. I am not sure if there is a cleaner way to do this.

This reverts commit 2d46d7f.

This reverts commit 1b798e5.

agriyakhetarpal · 2024-05-02T15:38:38Z

I think this is partially working now – where it breaks is when a try_examples.json config file is not present, and therefore data.ignore_patterns will always be false. I'll try to move the logic for un-hiding the buttons into a separate function and then call it. This way, the currently hidden buttons can be un-hidden with or without the presence of the configuration file.

agriyakhetarpal · 2024-05-02T18:18:53Z

I think this is working now, marking it as ready for review. One small thing to figure out remains: the PR preview at https://jupyterlite-sphinx--173.org.readthedocs.build/en/173/directives/try_examples.html is not un-hiding the button with the custom blue-bottom CSS present in jupyterlite_sphinx.css. The button is visible in the stable documentation: https://jupyterlite-sphinx.readthedocs.io/en/stable/directives/try_examples.html

So this works for all sorts of buttons now (tested locally), except for these non-standard buttons that have user-defined CSS. Let me try to dig further, and meanwhile open this up for further comments.

Carreau · 2024-07-22T08:30:10Z

I will try to re-review.

I'm ok with this in principle. Still unsure if hiding or appending is the best, so I'd like someone else input on this anyway.

agriyakhetarpal · 2024-12-04T00:06:19Z

I think this is working now, marking it as ready for review. One small thing to figure out remains: the PR preview at jupyterlite-sphinx--173.org.readthedocs.build/en/173/directives/try_examples.html is not un-hiding the button with the custom blue-bottom CSS present in jupyterlite_sphinx.css. The button is visible in the stable documentation: jupyterlite-sphinx.readthedocs.io/en/stable/directives/try_examples.html

So this works for all sorts of buttons now (tested locally), except for these non-standard buttons that have user-defined CSS. Let me try to dig further, and meanwhile open this up for further comments.

Update on this: With the refactoring I performed in my recent commits, I've fixed the problem I had noted above; however, using it on projects downstream, such as NumPy, fails, and I am not able to un-hide the buttons selectively based on the config file. So far, I still haven't been able to figure out why (it apparently has something to do with the CSS rather than with JS).

Even though this is marked as a draft, it's ready for review – @Carreau, could you please re-review it when you have time? I discussed this PR with @steppi, who agreed to review it soon after I suggested that I would revisit it, which I now have. Perhaps some trained pairs of eyes will be able to catch issues I haven't been able to :D

Also, a note on the jumping around of the contents when the page is loaded: I've tried to implement a solution (not pushed here, though, it should probably be a separate PR) to minimise the CLS value, through pre-allocating space for the container where the button is going to show up. However, for pages where the buttons are ignored/hidden, this check happens at runtime through the JavaScript code – which is not ideal. To achieve a smooth experience and no shifts in the content in the truest sense, this is possible only if we read through the documentation source pages and exclude them based on ignore patterns at build time (i.e., which would be read within the TryExamplesDirective class on the Python side), rather than at runtime on the JavaScript side, so that we avoid adding the HTML for the buttons altogether in the first place for pages where we need to ignore adding examples. I managed to get started on it, but I'm yet to test it (and I feel that should be in a separate PR, too).

gabalafou

Nice work to improve the user experience.

I agree that it's a very poor user experience to see a page with buttons and then see those buttons disappear inexplicably.

From that perspective, this pull request looks like it's doing the right thing.

However, if the paradigm is that most websites exclude only a small portion of pages, then this pull request penalizes the main case (include the page, show the buttons) in order to protect the exceptional case (ignore the page, hide the buttons).

I'm not sure there's really any good way to do this other than the one you mentioned—page exclusion patterns should be handled at build-time, not at run-time.

But we need to know why it was architected that way to begin with. There may be some really important use case for being able to dynamically add or remove exclusion patterns without having to rebuild the docs.

I'm requesting changes because the CSS should not have an !important flag, the toggle function needs fixing I think, and the JavaScript should have less copy-pasted code by creating showButtons() function.

gabalafou · 2024-12-19T07:11:30Z

jupyterlite_sphinx/jupyterlite_sphinx.js

+        const buttons = document.getElementsByClassName(
+          "try_examples_button hidden",
+        );


It's more common to write:

Suggested change

const buttons = document.getElementsByClassName(

"try_examples_button hidden",

);

const buttons = document.querySelectorAll(

".try_examples_button.hidden",

);

gabalafou · 2024-12-19T07:12:24Z

jupyterlite_sphinx/jupyterlite_sphinx.js

+      const buttons = document.getElementsByClassName(
+        "try_examples_button hidden",
+      );


Suggested change

const buttons = document.getElementsByClassName(

"try_examples_button hidden",

);

const buttons = document.querySelectorAll(

".try_examples_button.hidden",

);

gabalafou · 2024-12-19T08:01:46Z

jupyterlite_sphinx/jupyterlite_sphinx.css

+
+/* Some improvements for hidden buttons */
+
+/* smooth transitions */
+.try_examples_button {
+  opacity: 1;
+  transition: opacity 0.3s ease-in-out;
+}
+
+.try_examples_button.hidden {
+  display: none;
+  opacity: 0;
+}
+
+.try_examples_button.fade-in {
+  transition: opacity 0.3s ease-in-out;
+}
+
+/* to ensure hidden class takes precedence */
+.hidden {
+  display: none !important;
+}


I'm not entirely sure that it's worth creating the fade-in effect when there is also a content layout shift.

But if we still want to keep it, I suggest a few changes:

Suggested change

/* Some improvements for hidden buttons */

/* smooth transitions */

.try_examples_button {

opacity: 1;

transition: opacity 0.3s ease-in-out;

}

.try_examples_button.hidden {

display: none;

opacity: 0;

}

.try_examples_button.fade-in {

transition: opacity 0.3s ease-in-out;

}

/* to ensure hidden class takes precedence */

.hidden {

display: none !important;

}

/* Buttons are hidden by default and revealed via JavaScript in another file */

/* The button is revealed in two stages. The first stage removes the hidden class.

This gives the button dimensions and causes the page content below to "jump"

(get pushed down suddenly). The second stage removes the transparent class,

which reveals the button with a slight fade-in effect. */

.try_examples_button.transparent {

opacity: 0;

}

.try_examples_button {

opacity: 1;

transition: opacity 0.3s ease-in-out;

}

gabalafou · 2024-12-19T08:07:50Z

jupyterlite_sphinx/jupyterlite_sphinx.js

+        requestAnimationFrame(() => {
+          for (let button of buttons) {
+            button.style.opacity = "0";
+            button.classList.remove("hidden");
+            button.classList.add("fade-in");
+            requestAnimationFrame(() => {
+              button.style.opacity = "1";
+            });
+          }
+        });


Let's wrap this in a function:

const showButtons = () => { const buttons = document.querySelectorAll( ".try_examples_button.hidden", ); for (let button of buttons) { button.classList.remove("hidden"); } // If the hidden and transparent classes are removed // one right after the other, there is no animation of the opacity // going from 0 to 1, hence requestAnimationFrame. requestAnimationFrame(() => { for (let button of buttons) { button.classList.remove("transparent"); } }); }

gabalafou · 2024-12-19T08:10:44Z

jupyterlite_sphinx/jupyterlite_sphinx.js

+      const buttons = document.getElementsByClassName(
+        "try_examples_button hidden",
+      );
+      for (let button of buttons) {
+        button.classList.remove("hidden");
+      }


Suggested change

const buttons = document.getElementsByClassName(

"try_examples_button hidden",

);

for (let button of buttons) {

button.classList.remove("hidden");

}

showButtons();

gabalafou · 2024-12-19T08:18:34Z

jupyterlite_sphinx/jupyterlite_sphinx.py

        go_back_button_html = (
-            '<button class="try_examples_button" '
+            '<button class="try_examples_button hidden" '


From what I can tell, these buttons are already within a .hidden container.

Suggested change

'<button class="try_examples_button hidden" '

'<button class="try_examples_button" '

gabalafou · 2024-12-19T08:18:46Z

jupyterlite_sphinx/jupyterlite_sphinx.py

            f"onclick=\"window.tryExamplesHideIframe('{examples_div_id}',"
            f"'{iframe_parent_div_id}')\">"
            "Go Back</button>"
        )

        full_screen_button_html = (
-            '<button class="try_examples_button" '
+            '<button class="try_examples_button hidden" '


Suggested change

'<button class="try_examples_button hidden" '

'<button class="try_examples_button" '

gabalafou · 2024-12-19T08:20:24Z

jupyterlite_sphinx/jupyterlite_sphinx.py

@@ -570,7 +571,7 @@ def run(self):
        # Button with the onclick event to swap examples with embedded notebook.
        try_it_button_html = (
            '<div class="try_examples_button_container">'
-            '<button class="try_examples_button" '
+            '<button class="try_examples_button hidden" '


Suggested change

'<button class="try_examples_button hidden" '

'<button class="try_examples_button hidden transparent" '

(see other suggestions above)

gabalafou · 2024-12-19T08:35:21Z

jupyterlite_sphinx/jupyterlite_sphinx.js

  }
  tryExamplesConfigLoaded = true;
 };

 window.toggleTryExamplesButtons = () => {
  /* Toggle visibility of TryExamples buttons. For use in console for debug
   * purposes. */
-  var buttons = document.getElementsByClassName("try_examples_button");
+  var buttons = document.getElementsByClassName("try_examples_button hidden");


This is a toggle function, so you need to be able to find toggle any button whether or not it is already hidden.

Suggested change

var buttons = document.getElementsByClassName("try_examples_button hidden");

var buttons = document.getElementsByClassName("try_examples_button");

gabalafou · 2024-12-19T08:36:30Z

jupyterlite_sphinx/jupyterlite_sphinx.js

  }
  tryExamplesConfigLoaded = true;
 };

 window.toggleTryExamplesButtons = () => {
  /* Toggle visibility of TryExamples buttons. For use in console for debug
   * purposes. */
-  var buttons = document.getElementsByClassName("try_examples_button");
+  var buttons = document.getElementsByClassName("try_examples_button hidden");

  for (var i = 0; i < buttons.length; i++) {
    buttons[i].classList.toggle("hidden");


Suggested change

buttons[i].classList.toggle("hidden");

buttons[i].classList.toggle("hidden");

requestAnimationFrame(() => {

buttons[i].classList.toggle("transparent");

});

agriyakhetarpal force-pushed the make-buttons-hidden-by-default branch from d63d2e5 to 0ed1b1b Compare May 1, 2024 16:47

agriyakhetarpal added 3 commits May 1, 2024 22:17

Ignore VS Code folder

1b798e5

Set hidden to try_examples_button classes

37fa0ca

Un-hide TryExamples buttons based on regex

09d48ab

agriyakhetarpal force-pushed the make-buttons-hidden-by-default branch from 0ed1b1b to 09d48ab Compare May 1, 2024 16:48

Fix incorrect regex check

2d46d7f

Carreau reviewed May 2, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

jupyterlite_sphinx/jupyterlite_sphinx.js Show resolved Hide resolved

agriyakhetarpal added 3 commits May 2, 2024 18:58

Revert "Fix incorrect regex check"

0e88cbf

This reverts commit 2d46d7f.

Revert "Ignore VS Code folder"

d2811f9

This reverts commit 1b798e5.

Update toggleTryExamplesButtons function too

0745252

Update logic for tryExamplesConfigLoaded

a9c73ba

agriyakhetarpal changed the title ~~[WIP]: Hide buttons from TryExamples directive by default~~ Hide buttons from TryExamples directive by default May 2, 2024

agriyakhetarpal marked this pull request as ready for review May 2, 2024 18:18

jtpio added the enhancement New feature or request label May 3, 2024

Carreau self-assigned this Jul 22, 2024

agriyakhetarpal added 5 commits July 22, 2024 23:37

Merge branch 'main' into make-buttons-hidden-by-default

94ecef4

Add note about "hidden" class for TryExamples buttons

d015b5f

Refactor + fade examples buttons into the screen

635a3af

Merge branch 'main' into make-buttons-hidden-by-default

33b9f4e

Add smooth transitions

f3ca349

agriyakhetarpal changed the title ~~Hide buttons from TryExamples directive by default~~ Hide buttons generated by the TryExamples directive by default Dec 3, 2024

agriyakhetarpal marked this pull request as draft December 3, 2024 03:46

agriyakhetarpal requested review from steppi and Carreau December 4, 2024 00:06

gabalafou suggested changes Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide buttons generated by the `TryExamples` directive by default #173

Hide buttons generated by the `TryExamples` directive by default #173

agriyakhetarpal commented May 1, 2024

agriyakhetarpal commented May 1, 2024

Carreau left a comment

agriyakhetarpal commented May 2, 2024

agriyakhetarpal commented May 2, 2024

agriyakhetarpal commented May 2, 2024 •

edited

Loading

Carreau commented Jul 22, 2024

agriyakhetarpal commented Dec 4, 2024

gabalafou left a comment

gabalafou Dec 19, 2024

gabalafou Dec 19, 2024

gabalafou Dec 19, 2024

gabalafou Dec 19, 2024

gabalafou Dec 19, 2024

gabalafou Dec 19, 2024

gabalafou Dec 19, 2024

gabalafou Dec 19, 2024

gabalafou Dec 19, 2024

gabalafou Dec 19, 2024

	'<button class="try_examples_button hidden" '
	'<button class="try_examples_button" '

	var buttons = document.getElementsByClassName("try_examples_button hidden");
	var buttons = document.getElementsByClassName("try_examples_button");

Hide buttons generated by the TryExamples directive by default #173

Are you sure you want to change the base?

Hide buttons generated by the TryExamples directive by default #173

Conversation

agriyakhetarpal commented May 1, 2024

Description

Changes made

Additional context

agriyakhetarpal commented May 1, 2024

Carreau left a comment

Choose a reason for hiding this comment

agriyakhetarpal commented May 2, 2024

agriyakhetarpal commented May 2, 2024

agriyakhetarpal commented May 2, 2024 • edited Loading

Carreau commented Jul 22, 2024

agriyakhetarpal commented Dec 4, 2024

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hide buttons generated by the `TryExamples` directive by default #173

Hide buttons generated by the `TryExamples` directive by default #173

agriyakhetarpal commented May 2, 2024 •

edited

Loading