ENH: add optional `extra_classes` in version-switcher #576

tupui · 2022-01-20T12:28:33Z

Closes #566

Allow to specify additional classes per version in switcher.json.

Right now it adds the classes for all the versions. So a user might want to combine this with "active" if it's purely for styling otherwise it could make a Christmas tree in the dropdown. I could move the logic into the version check, but I was not sure if there was other use cases this could be used for.

tupui · 2022-01-20T12:38:22Z

Failure in CI seems unrelated.

drammock · 2022-01-24T17:29:31Z

This is different from what I was expecting, which was that the extra classes get added to the parent node (the drop-down button, not the individual entries). It will require checking whether the version in entry is the "current" version being viewed. The idea was that, e.g., the drop-down button could be red on outdated docs and green on stable and yellow on dev. But in all cases the drop-down entries have the same set of classes.

tupui · 2022-01-24T20:04:10Z

This is different from what I was expecting, which was that the extra classes get added to the parent node (the drop-down button, not the individual entries). It will require checking whether the version in entry is the "current" version being viewed. The idea was that, e.g., the drop-down button could be red on outdated docs and green on stable and yellow on dev. But in all cases the drop-down entries have the same set of classes.

I thought about the same and the reason I did it like this for now is that I was not sure if there might be other things people would want to do with this feature. You could add different markers, or shades, hide some elements based on some other context, etc.

I can move the block to the active only section so that it only applies there.

@larsoner @jorisvandenbossche what do you think?

jorisvandenbossche · 2022-01-24T21:14:58Z

I was also assuming this was about the dropdown "button", not entries. See also my comment at scipy/scipy#15380 (comment)

If we all initially thought about a class for the button, then I would propose to first tackle that. We can still later see if there is demand for classes in the dropdown entries, but I would personally wait until there are actual use cases.

tupui · 2022-01-24T22:00:05Z

I updated the PR to only apply the classes when an element is active.

jorisvandenbossche · 2022-01-25T08:39:26Z

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/_templates/version-switcher.html

+                // Add extra class to the entry
+                if ("extra_classes" in entry) {
+                    $.each(entry.extra_classes, function(index, extra_class) {
+                      node.classList.add(extra_class);


This is still adding the class to the entry link, I think? (although now only to the active version)

if I understand correctly, the current code will add the class in the html (as it is after being generated with the javascript, simplified a bit for explanation purposes) where I labeled it "HERE" (so in the first <a> node):

<div class="dropdown show" id="version_switcher"> <button type="button" class="..." id="version_switcher_button" ...> 0.8.0 <span class="caret"></span> </button> <div id="version_switcher_menu" class="dropdown-menu ..." ...> <a class="list-group-item ... HERE" href="https://pydata-sphinx-theme.readthedocs.io/en/v0.7.1/index.html">v0.8.0 (stable)</a> <a class="list-group-item ..." href="https://pydata-sphinx-theme.readthedocs.io/en/v0.7.1/index.html">v0.7.1</a> <a class="list-group-item ..." href="https://pydata-sphinx-theme.readthedocs.io/en/v0.7.0/index.html" >0.7.0</a > ... </div> </div>

While my understanding is that we want to be able to color the dropdown button, so have it instead at this location:

<div class="dropdown show" id="version_switcher"> <button type="button" class="... HERE" id="version_switcher_button" ...> 0.8.0 <span class="caret"></span> </button> <div id="version_switcher_menu" class="dropdown-menu ..." ...> <a class="list-group-item ..." href="https://pydata-sphinx-theme.readthedocs.io/en/v0.7.1/index.html">v0.8.0 (stable)</a> <a class="list-group-item ..." href="https://pydata-sphinx-theme.readthedocs.io/en/v0.7.1/index.html">v0.7.1</a> <a class="list-group-item ..." href="https://pydata-sphinx-theme.readthedocs.io/en/v0.7.0/index.html" >0.7.0</a > ... </div> </div>

ah ok I see. I move the code to version_switcher_button.

The changes are in. Can you have another look @jorisvandenbossche?

damianavila · 2022-01-27T13:43:16Z

+1, but wondering if we should showcase this on our site...

The idea was that, e.g., the drop-down button could be red on outdated docs and green on stable and yellow on dev. But in all cases the drop-down entries have the same set of classes.

Related to #574

tupui · 2022-01-27T13:45:56Z

+1, but wondering if we should showcase this on our site...

The idea was that, e.g., the drop-down button could be red on outdated docs and green on stable and yellow on dev. But in all cases the drop-down entries have the same set of classes.

Related to #574

@damianavila do you prefer that I copy this phrasing over what I put now in the doc?

extra_classes: an optional list of classes to add to the switcher
button for a given version (e.g., ["dev", "rc"]). These classes are only
added when the version is active.

EDIT: ok understood what you meant 😅 The demo site should show this.

jorisvandenbossche · 2022-01-27T14:01:56Z

wondering if we should showcase this on our site...

Ideally yes, but I think that will require #574 to first be merged

tupui · 2022-01-27T15:17:42Z

wondering if we should showcase this on our site...

Ideally yes, but I think that will require #574 to first be merged

@jorisvandenbossche If this PR goes in first, you can showcase the behaviour in #574

tupui · 2022-02-07T12:39:39Z

@drammock @jorisvandenbossche @damianavila can you please take another look?

choldgraf

This looks good to me - just one quick comment in there in case people give a string, but this looks good to me when that is resolved

tupui · 2022-02-08T11:31:32Z

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/_templates/version-switcher.html

@@ -72,6 +72,12 @@
            if (entry.version == "{{ theme_switcher.get('version_match') }}") {
                node.classList.add("active");
                $("#version_switcher_button").text(entry.name);
+                // Add extra class to the button for the selected entry
+                if ("extra_classes" in entry) {
+                    $.each(entry.extra_classes, function(index, extra_class) {


Personally I would not add complexity here. But I can make the change if you want.

I think the suggestion is not correct as the type check should happen before iterating on the list of classes. (But I am really a novice on JS so I could be missing something as well)

OK sounds good, let me re-work this so it's a one-liner then

tupui · 2022-02-08T11:31:32Z

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/_templates/version-switcher.html

@@ -72,6 +72,12 @@
            if (entry.version == "{{ theme_switcher.get('version_match') }}") {
                node.classList.add("active");
                $("#version_switcher_button").text(entry.name);
+                // Add extra class to the button for the selected entry
+                if ("extra_classes" in entry) {
+                    $.each(entry.extra_classes, function(index, extra_class) {


Personally I would not add complexity here. But I can make the change if you want.

I think the suggestion is not correct as the type check should happen before iterating on the list of classes. (But I am really a novice on JS so I could be missing something as well)

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/_templates/version-switcher.html

choldgraf

OK I added in a quick one-liner replacement to make it simpler.

I'm OK merging this in unless anybody else objects. But I'll note that I don't feel good about merging in a PR that we cannot test in our test suite, and that we cannot demo in our demo documentation. :-/

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/_templates/version-switcher.html

…ersion-switcher.html Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

tupui · 2022-02-08T21:14:40Z

Thanks for reviewing @choldgraf.

OK I added in a quick one-liner replacement to make it simpler.

I'm OK merging this in unless anybody else objects. But I'll note that I don't feel good about merging in a PR that we cannot test in our test suite, and that we cannot demo in our demo documentation. :-/

I guess you (I am not sure I would know what to do but I can try 😅) can update your PR #584 when this goes in. Also when this is in, #574 can be updated to reflect the last 2 PRs.

tupui · 2022-02-09T20:43:16Z

I resolved the conflicts. Had a quick look at the test you added @choldgraf but I don't understand how to adjust it for this PR (if I can since you added a note about not testing the JS).

choldgraf · 2022-02-09T21:10:41Z

Let's just merge this in, and add tests in the future once we figure out the right infrastructure. Maybe @jorisvandenbossche would be willing to add in some "extra classes" in #574 to test this out, and we can make a patch PR if something isn't working as expected.

choldgraf

Thanks @tupui for the improvement!

tupui · 2022-02-09T21:12:24Z

Sounds good, thanks 😃 !! Can't wait for the next release to use this.

ENH: add optional extra_classes in version-switcher

86f6d64

MAINT: move extra_classes to active version only

e98bd62

tupui force-pushed the extra_class_switcher branch from 1bc50f3 to e98bd62 Compare January 24, 2022 21:19

jorisvandenbossche reviewed Jan 25, 2022

View reviewed changes

MAINT: move extra_classes to button

d7616a9

tupui force-pushed the extra_class_switcher branch 2 times, most recently from 8b5f2bb to f168b98 Compare January 25, 2022 11:24

DOC: add extra_classes in one example

c50de2d

tupui force-pushed the extra_class_switcher branch from f168b98 to c50de2d Compare January 25, 2022 11:27

tupui requested a review from jorisvandenbossche February 7, 2022 12:38

choldgraf approved these changes Feb 8, 2022

View reviewed changes

tupui commented Feb 8, 2022

View reviewed changes

choldgraf mentioned this pull request Feb 8, 2022

TEST: Basic test for our switcher #584

Merged

choldgraf reviewed Feb 8, 2022

View reviewed changes

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/_templates/version-switcher.html Outdated Show resolved Hide resolved

choldgraf approved these changes Feb 8, 2022

View reviewed changes

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/_templates/version-switcher.html Outdated Show resolved Hide resolved

Update src/pydata_sphinx_theme/theme/pydata_sphinx_theme/_templates/v…

822536f

…ersion-switcher.html Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

Merge branch 'master' into extra_class_switcher

77224e0

choldgraf approved these changes Feb 9, 2022

View reviewed changes

choldgraf merged commit c987276 into pydata:master Feb 9, 2022

tupui deleted the extra_class_switcher branch February 9, 2022 21:12

damianavila mentioned this pull request Feb 22, 2022

Bump to dev version + update versions in switcher.json #574

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: add optional `extra_classes` in version-switcher #576

ENH: add optional `extra_classes` in version-switcher #576

tupui commented Jan 20, 2022

tupui commented Jan 20, 2022

drammock commented Jan 24, 2022

tupui commented Jan 24, 2022

jorisvandenbossche commented Jan 24, 2022

tupui commented Jan 24, 2022

jorisvandenbossche Jan 25, 2022

tupui Jan 25, 2022

tupui Jan 26, 2022

damianavila commented Jan 27, 2022

tupui commented Jan 27, 2022 •

edited

Loading

jorisvandenbossche commented Jan 27, 2022

tupui commented Jan 27, 2022

tupui commented Feb 7, 2022

choldgraf left a comment

tupui Feb 8, 2022

choldgraf Feb 8, 2022

tupui Feb 8, 2022

choldgraf left a comment

tupui commented Feb 8, 2022

tupui commented Feb 9, 2022

choldgraf commented Feb 9, 2022

choldgraf left a comment

tupui commented Feb 9, 2022

ENH: add optional extra_classes in version-switcher #576

ENH: add optional extra_classes in version-switcher #576

Conversation

tupui commented Jan 20, 2022

tupui commented Jan 20, 2022

drammock commented Jan 24, 2022

tupui commented Jan 24, 2022

jorisvandenbossche commented Jan 24, 2022

tupui commented Jan 24, 2022

jorisvandenbossche Jan 25, 2022

Choose a reason for hiding this comment

tupui Jan 25, 2022

Choose a reason for hiding this comment

tupui Jan 26, 2022

Choose a reason for hiding this comment

damianavila commented Jan 27, 2022

tupui commented Jan 27, 2022 • edited Loading

jorisvandenbossche commented Jan 27, 2022

tupui commented Jan 27, 2022

tupui commented Feb 7, 2022

choldgraf left a comment

Choose a reason for hiding this comment

tupui Feb 8, 2022

Choose a reason for hiding this comment

choldgraf Feb 8, 2022

Choose a reason for hiding this comment

tupui Feb 8, 2022

Choose a reason for hiding this comment

choldgraf left a comment

Choose a reason for hiding this comment

tupui commented Feb 8, 2022

tupui commented Feb 9, 2022

choldgraf commented Feb 9, 2022

choldgraf left a comment

Choose a reason for hiding this comment

tupui commented Feb 9, 2022

ENH: add optional `extra_classes` in version-switcher #576

ENH: add optional `extra_classes` in version-switcher #576

tupui commented Jan 27, 2022 •

edited

Loading