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

BUG: Fix links not clickable in versionmodified directives #877

Merged
merged 2 commits into from Aug 18, 2022

Conversation

AnirudhDagar
Copy link
Contributor

@AnirudhDagar AnirudhDagar commented Aug 16, 2022

Reference scipy/scipy#16846 4th point.

https://pydata-sphinx-theme.readthedocs.io/en/stable/demo/theme-elements.html#version-changes uses the pseudo content CSS property to add the background colour for different version modified directives (versionadded, versionchanged, deprecated), unfortunately it seems like it is on top of the text content (higher in z-index). This leads to text not being clickable; more specifically, all links in the text are blocked and don't work. This is quite a problem because usually when deprecating a method, you would want to link the alternate method from the docs in the message.

See here as an example of the links not working.

This PR fixes the issue. I'm not a CSS expert, and there are many ways to fix the problem. Let me know if you have a better idea or if this change could break something else (i hope not).

cc @tupui @rgommers

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

I didn't know this css property. Let me check in dummy documentation but in principle, I find it very elegant.

@tupui
Copy link
Contributor

tupui commented Aug 16, 2022

Thank you for opening this @AnirudhDagar. Interesting solution indeed 🙌

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

tested from my side and it works like a charm, link are visible, the cursor is changing and links are clickable

Capture d’écran 2022-08-17 à 23 02 57

@AnirudhDagar
Copy link
Contributor Author

Thanks @12rambau for the review and testing! Is there any chance for this to be included with the next release? I see that the v0.10.0rc2 is already out. Will there be an rc3 after the build speed issues are fixed?

@12rambau
Copy link
Collaborator

pinging @choldgraf as I'm not managing the releases

@tupui
Copy link
Contributor

tupui commented Aug 17, 2022

@AnirudhDagar this is not like SciPy where there is a release branch and we need to constantly backport things. The state on main is going to be the release state when a release is made. So if this gets in before the release (most likely), this will be in as well.

@choldgraf
Copy link
Collaborator

This looks great to me - I just pushed a quick commit that removes the pointer-events rule from our "versionadded" etc rules, and instead inserts it one time into the mixin which is where this behavior comes from in the first place. This should mean that we also resolve this issue for our admonition titles etc as well. I hope that's OK with you @AnirudhDagar !

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good to me, and the hovering works properly with it in the include now...I can't imagine that the newly-failing audit test is a result of this change so I assume it's temporary. +1 on merge from me!

@12rambau 12rambau merged commit 8885596 into pydata:main Aug 18, 2022
@AnirudhDagar
Copy link
Contributor Author

The state on main is going to be the release state

@tupui thanks for the clarification!

Thanks @choldgraf for improving the code, yeah makes sense to include it at a higher level to fix other affected sections. 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants