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

Configure versions.json and remove the version switcher dropdown #3108

Merged

Conversation

agriyakhetarpal
Copy link
Member

Description

Adds a version banner at the top of the page for old and unstable versions, and configures the version switcher dropdown to work again

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all
  • The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (695917e) 99.71% compared to head (2989bb3) 99.71%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3108   +/-   ##
========================================
  Coverage    99.71%   99.71%           
========================================
  Files          248      248           
  Lines        18764    18764           
========================================
  Hits         18710    18710           
  Misses          54       54           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@agriyakhetarpal
Copy link
Member Author

For some reason, the switcher dropdown on readthedocs has disappeared for all our versions and does not work locally either. I double-checked the PyData theme documentation and our configuration, and compared it with that of NumPy and SciPy too just in case I may have missed something.

I can confirm that the switcher was working earlier, however: #2769 (comment). It could be due to a related JS file not loading itself when it should (i.e., upon clicking the switcher). I will keep this a draft for now until I am able to dig further 😕

@agriyakhetarpal
Copy link
Member Author

I see that you approved this @tinosulzer, but it is not ready to merge yet, let's keep this open

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Jul 18, 2023

Marking this as ready for review since I think it would make sense to wait for pydata/pydata-sphinx-theme#1354 to get the warning bar from the theme itself; following a discussion with one of the theme maintainers. We may discuss this in this week's meeting

We should add the warning bar when the theme has a new release and configure versions.json before our 23.9 release in a separate PR later, so that the currently stable build (23.5) will start displaying the version warning banner when it becomes an old release. Meanwhile, I would suggest merging this PR so that it does not become outdated afterwards lest it is kept open

@agriyakhetarpal agriyakhetarpal changed the title Add a loud version banner and fix versions dropdown Configure versions.json and fix the version switcher dropdown Jul 18, 2023
@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review July 18, 2023 15:08
@agriyakhetarpal
Copy link
Member Author

The failing Windows tests will be fixed by #3157

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @agriyakhetarpal! I am guessing that there are no blockers for this PR now? You might also want to merge in develop.

@agriyakhetarpal
Copy link
Member Author

No blockers, but I am yet to be convinced whether the logic I wrote for setting the switcher is correct. I would say we should wait for a while

docs/conf.py Outdated
Comment on lines 186 to 188
if version_type == "external":
# Set the version to latest on readthedocs PRs
html_theme_options["switcher"]["version_match"] = "latest"
Copy link

Choose a reason for hiding this comment

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

This will only set the html_theme_options["switcher"]["version_match"], it won't work for stable nor latest. Currently, they'll get version = pybamm.__version__ (defined in line 33) which is not the version you want to have matched to them IUUC. In addition to these checks to cover local and PR builds, I think you should also have specific checks for stable and latest. You can use the version_match = os.environ.get("READTHEDOCS_VERSION") variable which you have defined a few lines higher. According to rtd docs it will take exactly the values latest/stable for their builds.

They do have independent builds and they appear on the versions.json file, so their builds should have the right value of the version match. Otherwise, users will click on latest and go to .../en/latest/... docs, but the "folded" version switcher won't show latest while browsing the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review! As noted previously in our conversation—you mentioned that the version switcher is still not going to work for previous documentation releases, so we are deciding to remove the dropdown altogether in order to rely on the server-side readthedocs one. That is, unless there exists a workaround for not having to deal with patch releases with the latest versions.json set?

@agriyakhetarpal agriyakhetarpal changed the title Configure versions.json and fix the version switcher dropdown Configure versions.json and remove the version switcher dropdown Jul 28, 2023
@agriyakhetarpal
Copy link
Member Author

@Saransh-cpp let's get this in; I don't think we can get the switcher on older versions anyway (pydata/pydata-sphinx-theme#1389 (comment)). The next step after this is merged will be to add the theme's version warning bar that will come with their next release, before our release

@Saransh-cpp Saransh-cpp merged commit 08a0ea1 into pybamm-team:develop Jul 28, 2023
20 of 21 checks passed
@agriyakhetarpal agriyakhetarpal deleted the version-banner-switcher branch July 28, 2023 14:36
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