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

Implement hoverxref for floating tooltips #2290

Merged
merged 26 commits into from
Nov 25, 2024
Merged

Conversation

RDaxini
Copy link
Contributor

@RDaxini RDaxini commented Nov 5, 2024

  • Partially closes Glossary page updates tracking #2284; related comment
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

See example here by hovering your cursor over the term "dni"

@kandersolar
Copy link
Member

@RDaxini I suggest adding that sphinx plugin as a new doc dependency here: https://github.com/pvlib/pvlib-python/blob/main/pyproject.toml#L57-L69

@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 5, 2024

@kandersolar You can see me struggling to find out where to add it 😂
I was trying to find to find where doc list of dependencies is but failed... then got a bit muddled up with other random attempts. I should have just asked sooner... 😂
Thanks!

@RDaxini RDaxini marked this pull request as ready for review November 6, 2024 00:02
@RDaxini RDaxini changed the title [WIP] Implement hoverxref for floating tooltips (definitions) Implement hoverxref for floating tooltips (definitions) Nov 6, 2024
@kandersolar kandersolar added this to the v0.11.2 milestone Nov 6, 2024
@kandersolar
Copy link
Member

I like it! As we eventually make wider use of :term: throughout the docs, I think this will be a really nice feature.

I suppose we should consider enabling it for more than just :term:. For example, it should also work for :func: and friends. I think I'd lean towards enabling it just for :term: for now, but I wonder what other people think.

@AdamRJensen
Copy link
Member

I suppose we should consider enabling it for more than just :term:. For example, it should also work for :func: and friends. I think I'd lean towards enabling it just for :term: for now, but I wonder what other people think.

What drawbacks would there be to enabling it foe everything immediately? I think I'm leaning that way

@kandersolar
Copy link
Member

I guess the downside I see is that function docstrings are much larger and thus more "intrusive" as a pop-up. I guess there's no harm in trying it out in this PR just to see what it's like!

@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 9, 2024

  1. We don't have any instances of :func: :class: or :meth: , right? Are these always :py: ... ?
    Quick scan of the results of git grep -n -F ":func:" (same for :meth:, and :class:) suggests so
  2. Don't think there is a way to generate tooltips for our citations since we don't use bibtex with the :cite: role, but let me know if you know of one.

@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 9, 2024

Looks like :term: and :ref: are working, but no luck with :py:func: yet. e.g. see the warning in irradiance.perez. Any ideas?

@echedey-ls
Copy link
Contributor

This improvement is super :D

  1. Are these always :py: ... ?

Yes. That prefix is for the domain https://www.sphinx-doc.org/en/master/usage/domains/index.html
Just in case someone want to rewrite pvlib in Julia or Rust, I guess we can keep using it even thou it's on by default.

My eyes hurt after seeing this:
image

Issue reported at readthedocs/sphinx-hoverxref#231
Maybe you can do something along pybamm-team/PyBaMM#3083
Personal taste of adding the style in a separate CSS file. At first glance, it should work as they also use pydata-sphinx-theme, and the main problem is that sphinx-hoverxref can't deduce the exact names of the colours used by each style.

Mitigation for my fellas at https://darkreader.org/

@RDaxini RDaxini changed the title Implement hoverxref for floating tooltips (definitions) Implement hoverxref for floating tooltips Nov 14, 2024
@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 14, 2024

Trying 2bea511 to enable tooltips for links to projects not hosted on readthedocs e.g. scipy (example: notes section on this page)

docs/sphinx/source/conf.py Show resolved Hide resolved
docs/sphinx/source/conf.py Outdated Show resolved Hide resolved
@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 15, 2024

To do:

  • dark theme compatibility
  • (potential) tooltip sizing limits

Let me know if there's anything else or if you have any thoughts on the items above

@RDaxini RDaxini mentioned this pull request Nov 18, 2024
7 tasks
@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 18, 2024

  • dark theme compatibility

do I need to create a new .css file to contain something along these lines: https://github.com/pybamm-team/PyBaMM/pull/3083/files#diff-faffc787a692ac8c3c013159ca171e14a027d0417bdec68d8b3624dbb4aee0f0R153-R169
I see we have a couple of .css files in sphinx\source\_static but not sure how to interpret them. Can someone point me in the direction of an online source to help me understand this aspect of pvlib?

@echedey-ls
Copy link
Contributor

@RDaxini you are right in all your points.

Specifically, all CSS files get sent to the browser and it applies the rules/styles in them. It would be certainly equivalent to add the same styles in an existing file, but I think it's more maintainable if you add that to another file.

Documentation in conf.py
Sphinx docs: https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_static_path

Just in case, although you probably won't need to learn much about CSS, this is my go-to sources of web documentation

And if you want to play a bit, you can start by modifying the webpage live through your browser inspector (usually opens from Ctrl+Shift+C, at least in chrome and firefox).

@kandersolar
Copy link
Member

A general comment: CSS tweaks target parts of these packages which are not really part of the "public API" and often stop working when we update to newer versions. There is a place for CSS tweaks in our docs, but it's best to keep them minimal. It would be much better if it could be fixed upstream instead of here (nudge @echedey-ls :P).

dark theme compatibility

What do you guys think about merging this PR the way it is now, and looking into fixes for the dark mode thing in a follow-up? If the dark mode problem can be fixed easily then great, but I think it is acceptable (even if suboptimal) the way it is now too.

(potential) tooltip sizing limits

What is this one about?

@echedey-ls
Copy link
Contributor

echedey-ls commented Nov 19, 2024

What do you guys think about merging this PR the way it is now

I agree. @RDaxini if you want I could have a try at it if you haven't started yet, thou if you wanna learn a bit about web development it's definitely a manageable task.

often stop working when we update to newer versions

Yes, I overlooked that. 'sphinx-hoverxref' in pyproject.toml should get pinned to whatever version you are using in your environment @RDaxini .

It would be much better if it could be fixed upstream instead of here (nudge @echedey-ls :P).

Haha, just another side-contribution to the lengthy list xD

@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 19, 2024

What do you guys think about merging this PR the way it is now, and looking into fixes for the dark mode thing in a follow-up? If the dark mode problem can be fixed easily then great, but I think it is acceptable (even if suboptimal) the way it is now too.

+1

(potential) tooltip sizing limits

What is this one about?

I think it is possible to limit the size of tooltips, for example if we thought that the tooltip for a page cross-reference was too large, we can implement a size restriction. I think the current sizing is fine, but it's an option if anyone feels strongly otherwise.

Might be easier to decide on these details after an initial implementation and increased used across the docs. Goes back to your first point about merging this first and then considering further developments.

@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 19, 2024

@echedey-ls thanks, yeah I am quite interested, but I guess it would take me longer and I might need some extra help 😅 If you guys prefer, I don't mind if someone else takes over, but otherwise I could give it a go 😅

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Let's go ahead with merging this, with the possibility of further improvements later on. Thanks @RDaxini and @echedey-ls!

@kandersolar kandersolar merged commit 00aabaa into pvlib:main Nov 25, 2024
26 checks passed
@RDaxini RDaxini deleted the hoverxref branch November 25, 2024 19:02
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.

Glossary page updates tracking
4 participants