Skip to content

DOC [PST] fix dropdowns #28401

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

Merged
merged 22 commits into from
Apr 15, 2024
Merged

Conversation

Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Feb 12, 2024

Please note that this PR targets the new_web_theme branch!

Towards #28084. This mainly intends to fix the dropdowns. Note that there are many changes not closely related to the dropdown implementation itself, but are needed either to solve sphinx errors or keep consistency. The core changes are in conf.py and sphinxext/dropdown_anchors.py. This page contains many dropdowns thus may be used as an example.

Here is the list of things left to be done in this PR (if any). Maintainers please feel free to modify.

  • Placeholder.

@ArturoAmorQ who is in charge of #26617
@GaelVaroquaux who implemented the current |details-start| approach
@lucyleeow who opened #27127
@glemaitre who created details-permalink.js

I'm not sure if pinging maintainers like this would be annoying (please definitely let me know if it is). But if you have time and are interested please take a look. Thank you very much!

Copy link

github-actions bot commented Feb 12, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 5761a2b. Link to the linter CI: here

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 12, 2024

Why using the sphinx-design directive?

I did try to tweak the CSS of our current implementation to achieve a nice visual effect, but the current implementation is after all a hack. If you look closely at the generated HTML there are all those empty p tags caused by the browser auto fixing I closed tags. Using a directive is the more "sphinx" way of doing things from my perspective, and easier to maintain.

Why all those changes to the doc files?

The topic directive is not allowed to be nested in other directives, e.g. dropdowns. Moreover, I believe we do not really mean "topic" in those cases; instead we just want a bold title. Rubrics might be better in this case. And for consistency, I made all the "examples" and "references" into rubrics, thus all those changes.

Why generating dropdown anchors statically instead of using JS?

First JS could be disabled, then the anchors will not work. Second this is tweaking HTML instead of something dynamic on the website that only JS can do. Overall I think it is worth trading build time for this.

@Charlie-XIAO Charlie-XIAO marked this pull request as ready for review February 12, 2024 04:07
Copy link
Member

@ArturoAmorQ ArturoAmorQ left a comment

Choose a reason for hiding this comment

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

Hi @Charlie-XIAO. Thanks for your time and dedication on improving the documentation. I don't think I have enough expertise to comment on the actual solution for the dropdowns but the result seems to work as intended.

In any case, here is a batch of small comments. And maybe now that you are working on such general maintenance, do you mind correcting the indentation in the (current) lines 448 to 462 from contributing.rst? They lead to a wrong rendering.

Comment on lines +11 to +13
Some of the dropdowns were originally headers that had automatic anchors, so we
need to make sure that the old anchors still work. See the original implementation
(in JS): https://github.com/scikit-learn/scikit-learn/pull/27409
Copy link
Member

Choose a reason for hiding this comment

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

I think we never really used the anchors. Maybe we can simplify the code under this assumption and test a posteriori with a link checker.

Copy link
Member

@ArturoAmorQ ArturoAmorQ Feb 13, 2024

Choose a reason for hiding this comment

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

I just noticed sphinx already has an anchor convention for repeated titles, for instance see

https://scikit-learn.org/stable/modules/ensemble.html#id27

so maybe we can keep their convention or something similar for the sake of compatibility. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well in fact I'm not even sure if this comment is the actual intention of details-permalink.js because it is not me who implemented it.

I just noticed sphinx already has an anchor convention for repeated titles, for instance see https://scikit-learn.org/stable/modules/ensemble.html#id27, so maybe we can keep their convention or something similar for the sake of compatibility. WDYT?

I'm not sure about this, maybe it should be up to maintainers to decide. From the implementation side, I need to find out a way to check all IDs that sphinx added automatically to avoid conflicts. And after second thought the current implementation may suffer from duplicated IDs as well because we only check our own IDs but not those added by sphinx or other extensions. This is a tricky one and I may need further investigation as well as comments from maintainers who I've pinged in the top comment.

Copy link
Member

Choose a reason for hiding this comment

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

We rely on sphinx warnings to detect duplicate IDs, so it should show up there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think sphinx is able to detect duplicated IDs added like this? For instance if I tried removing the anchor_id_counters and got no warnings.

@Charlie-XIAO
Copy link
Contributor Author

Thanks for your quick reply @ArturoAmorQ I will address your comments asap.

@adrinjalali
Copy link
Member

One discussion we had here, is how to make firefox be able to find content inside the dropdowns when they're not collapsed? Or how to have an easy way to collapse all drop downs.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 15, 2024

Ah I've never noticed that <details> is not supported by all browsers (if I did not misunderstand what you are saying). Then an easy alternative might be to use an additional extension sphinx-togglebutton which uses JavaScript instead of <details>.

If I misunderstood could you please specify a bit more or point me to related discussions?

@adrinjalali
Copy link
Member

Yes, that's what I mean I think. I'd be okay with the extension. But with the extension, I can't see if it allows users to collapse everything at once.

@Charlie-XIAO
Copy link
Contributor Author

Can you specify why this is needed? And I think refreshing the page collapses all dropdowns.

@adrinjalali
Copy link
Member

For two reasons:

  • be able to read the page w/o having to collapse everything
  • be able to search in the page, as is, search (in firefox) doesn't find content inside the drop downs.

@Charlie-XIAO
Copy link
Contributor Author

I see. So I experimented with Chrome, Edge, and Firefox, it seems like:

  • They all cannot look into the JavaScript dropdown (based on Bootstrap collapse) when it is hidden;
  • Firefox cannot look into <details> when it is not opened while the other two can.

For both JavaScript dropdown and <details> we can create links to collapse/expand all with just a few lines of JavaScript; perhaps we can put it/them in the secondary sidebar when there are dropdowns in the page? One other concern is when JavaScript is disabled:

  • With sphinx-togglebutton it will directly show all content when JavaScript is disabled so no problem;
  • With sphinx-design I don't see a good way to open all <details> when JavaScript is disabled (but after all this is a corner case I think?)

@adrinjalali
Copy link
Member

I don't mind not having a way to open all dropdowns when JS is disabled.

And we should probably use <details> since other browsers can search into them.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 16, 2024

I agree, so we stick to sphinx-design dropdowns. However, I'm not sure how to integrate this functionality into the theme. Candidates in my mind are:

  • An icon in the top navbar (but I haven't found an icon that is informative enough yet);
  • A component in the secondary sidebar saying "Hide/Show all dropdowns";
  • A keyboard shortcut to trigger.

WDYT or is there a better idea?

@adrinjalali
Copy link
Member

how hard would it be to add something to each drop down, the default one expands the drop down, there could be another one to collapse all, I'm really not sure which one is better.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 16, 2024

This is a nice idea, and it would not be too hard. But I tried a bit and failed to find a good icon to represent its meaning. For instance, something like the following is not very informative and looks a bit strange:

image

Do you have any recommendation, or maybe we can directly use text description? (In that case we need to make sure the text is not mistaken as the description for the original icon.)

On the other hand, after second thought I think there may be another possible solution. When hovering on any dropdown we show a floating button like this for a few seconds (only the chevrons are clickable):

image

Emm but this also does not look that good... and maybe it is a bit too noisy?

@adrinjalali
Copy link
Member

I quite like your first idea, I think those icons look self explanatory enough, and with a hint on hover it would be quite nice.

@GaelVaroquaux
Copy link
Member

I agree with @adrinjalali that the first idea is very good:
image
If you can make tooltips so that when we hover on them it explains what's going on, it would work well.

@Charlie-XIAO , you seem to have a nice feeling for web-based UX. It's a pleasure to see this!

@Charlie-XIAO
Copy link
Contributor Author

Thanks for your suggestions @adrinjalali @GaelVaroquaux! I implemented something like follows:

Contributing.scikit-learn.1.5.dev0.documentation.7.-.-.Microsoft.Edge.2024-02-17.20-57-56.mp4

@adrinjalali adrinjalali self-requested a review February 18, 2024 18:07
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Checked a few pages, and this looks great to me ❤️

Comment on lines +11 to +13
Some of the dropdowns were originally headers that had automatic anchors, so we
need to make sure that the old anchors still work. See the original implementation
(in JS): https://github.com/scikit-learn/scikit-learn/pull/27409
Copy link
Member

Choose a reason for hiding this comment

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

We rely on sphinx warnings to detect duplicate IDs, so it should show up there.

@glemaitre
Copy link
Member

I looked quickly at the generated page and I find the solution super neat. I plan to review some the PST PR this week. But the rendering of this one is good.

@Charlie-XIAO
Copy link
Contributor Author

I updated the logic of sphinxext/dropdown_anchors.py in 5c47c2c. Previously I inserted the anchor after the first text node, but then it would not work for the following case. The new logic inserts the anchor as the third last element (see the comment in that file).

051be4f8eccfabf2ab088c47c22c5e3

@ogrisel
Copy link
Member

ogrisel commented Feb 22, 2024

The CI is red because of the following sphinx warning:

Sphinx Warnings in affected files

    doc/modules/gaussian_process.rst:489: WARNING: duplicate citation RW2006, other instance in
    doc/auto_examples/gaussian_process/plot_gpr_co2.rst 

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I tested with chrome and I confirm that ctrl-f searching for terms in collapsed sections still work and and I also confirm that using the expand-all buttons in firefox is a good enough workaround for:

https://bugzilla.mozilla.org/show_bug.cgi?id=1724299

LGTM once the problem reported by the CI is fixed.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 22, 2024

The sphinx warning is strange: indeed there are multiple RW2006 but they are not in the same page, and there are plenty of other RW2006 around that do not cause such a warning. Perhaps I'm misunderstanding how explicit citation in rst works...

Regardless, using a footnote instead would be a simple workaround.

@adrinjalali
Copy link
Member

Merging since CI is green.

@adrinjalali adrinjalali merged commit c1adbac into scikit-learn:new_web_theme Apr 15, 2024
@Charlie-XIAO Charlie-XIAO deleted the pst-dropdown branch August 13, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants