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

DOC, DEP: Remove custom :button_text directive option, bump jupyterlite-sphinx #737

Merged

Conversation

agriyakhetarpal
Copy link
Collaborator

Description

This is a small PR because jupyterlite-sphinx has a new release (jupyterlite/jupyterlite-sphinx#171) which simplifies some things here.

Addresses some issues found in #728 and is a bigger part of #706. I had committed this last month, now rebased on top of the latest changes.

Changes made

  1. I updated the minimum jupyterlite-sphinx version to 0.14.0 corresponding to the new release, and
  2. with that, I removed the :button_text: option from the .. try_examples:: directives everywhere in the source code, since the directive can now use the global configuration in conf.py.

Additional context

While the documentation is building alright right now, the upper pin on docutils restricts us to Sphinx v5.3.0, while the seemingly more stable v7.3.7 is available. Should we remove this pin? I removed it and built the docs again without issues, so, it doesn't seem to be needed. @rgommers, do we still need this pin or can we relax it? I can't find anything with a quick search suggestive of why this pin exists; if it makes sense, I am happy to do this in the same PR.

@agriyakhetarpal agriyakhetarpal changed the title DOC: Remove custom :button_text directive option, bump jupyterlite-sphinx DOC, DEP: Remove custom :button_text directive option, bump jupyterlite-sphinx Apr 30, 2024
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @agriyakhetarpal! This LGTM and seems to render fine on the RTD preview, so let's give this a go.

The Sphinx pin: pretty sure it was because of something that broke in some recent version (6.x had a lot of changes). But if 7.3 works fine, sure let's upgrade to that. Perhaps let's make it the minimum requirement then, to skip 6.x. Better in a separate PR I think, because it's a separate issue. Could you open a new PR for it?

@rgommers rgommers merged commit c151e21 into PyWavelets:main May 1, 2024
16 checks passed
@rgommers rgommers added this to the v1.7.0 milestone May 1, 2024
@agriyakhetarpal agriyakhetarpal deleted the fix-directive-specific-button-text branch May 1, 2024 07:41
@@ -90,7 +90,6 @@ computations can be performed with the `periodization`_ mode:
**Example:**

.. try_examples::
:button_text: Try it in your browser!
Copy link
Member

Choose a reason for hiding this comment

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

@agriyakhetarpal on this page the buttons disappeared. Maybe something went wrong here, could you check please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's quite strange. I'm able to see the two buttons on https://pywavelets.readthedocs.io/en/latest/ref/signal-extension-modes.html, which is mapped to this file, and I verified that this file locally also contains two buttons. Could you please try with a fresh browser instance, or reload this page with caches disabled (Ctrl+Shift+R), perhaps?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, wrong page. I was showing this to someone else during a meeting, and we were looking at Usage Examples. I thought we had buttons there? It has a modes pages too: https://pywavelets.readthedocs.io/en/latest/regression/modes.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, the pages under the "Usage Examples" section need work to get them working at this time; just the API reference has been implemented. We shall need something like MyST-NB and then convert everything to notebooks in order for them to run with the NotebookLite directive. I reckon that Jupytext notebooks will be better than converting to IPyNB. I can get started on this soon if you would want me to do so? We had previously left these for a later time, now sounds like a good time to pick it up.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I misremembered then - that's what I get for doing things during a meeting on a Friday night.

No need to do it now if MyST-NB isn't ready yet (I'm aware of the work in SciPy on it), but once it's unblocked then yes it would be good to do it.

I reckon that Jupytext notebooks will be better than converting to IPyNB.

Oh yes, definitely some .md format and no .ipynb.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I misremembered then - that's what I get for doing things during a meeting on a Friday night.

No worries! 😄

No need to do it now if MyST-NB isn't ready yet (I'm aware of the work in SciPy on it), but once it's unblocked then yes it would be good to do it.

Oh yes, definitely some .md format and no .ipynb.

It's ready, actually, it is just SciPy that hits some edge cases because of its custom MathJax, and we managed to find a suitable solution quite recently. The rest of the work there can now be resumed, so nothing remains blocked. I think I will consider that, plus the relatively smaller size of PyWavelets, as my sign and start the work, after which all pages under the PyWavelets documentation will be embellished with interactivity.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great, thanks Agriya.

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.

2 participants