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

use project version (not theme version) in banner #1446

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

drammock
Copy link
Collaborator

fixes #1445

@vnmabus can you test against this branch?

@vnmabus
Copy link

vnmabus commented Sep 15, 2023

Sorry if this is a noob question but...

Is it possible to test the banner locally? Because currently the correct versions appear only when I apply the changes to the develop branch and it is compiled in ReadTheDocs (when I compile locally and in PR compilations in RTD, I only see the current version). I would prefer to try the branch locally, if possible.

@drammock
Copy link
Collaborator Author

yep! You can build your site locally (using sphinx-build) and then fire up a simple HTTP server using python -m http.server (after first changing directory to the folder where your built site goes). i.e., something like this:

$ sphinx-build docs/ docs/_build/html/
[... lots of output ...]
$ cd docs/_build/html
$ python -m http.server
Serving HTTP on 0.0.0.0 port 8000 (http://0.0.0.0:8000/) ...

@vnmabus
Copy link

vnmabus commented Sep 15, 2023

That does not solve the problem... there is still one version to select (and thus no banner) when I build locally.

@drammock
Copy link
Collaborator Author

What is the repo? (so I can clone locally and try it)

@12rambau
Copy link
Collaborator

@drammock
Copy link
Collaborator Author

drammock commented Sep 15, 2023

https://dcor.readthedocs.io/en/latest/?badge=latest I guess

🤦🏻 of course. Sorry. will check after lunch.

@drammock
Copy link
Collaborator Author

That does not solve the problem... there is still one version to select (and thus no banner) when I build locally.

It shouldn't matter that you're only building one version. What matters are:

  1. you can access the versions JSON file
  2. the version you're currently building is different from the "preferred" version

in the case of dcor, the version reported is "0.6" even on your develop branch:

https://github.com/vnmabus/dcor/blob/3baf2c253bd3f44b3f9f6e4644580b116685c8c7/dcor/__init__.py#L45

That would be enough to explain why the warning banner is not showing up locally for you. If you set it to something greater than your "preferred" version (like "0.7.dev0") then it should work locally... except that your local site is trying to fetch the JSON file from remote source, so in most browsers it will be blocked by CORS policy. So to really test it locally you need to (temporarily) disable CORS in your browser. This is notoriously hard to do in Firefox (there are addons, but they often work for a while and then break with new FF releases). With chrome it should work to launch from a terminal like chrome --disable-web-security

@vnmabus
Copy link

vnmabus commented Sep 15, 2023

I was able to try the banner locally with the pointers you gave me. I checked that with this PR the version displayed is no longer the theme version. So far so good.

However, I checked the banner by setting the version_match attribute manually to 0.1. In the dropdown it appears correctly as 0.1. However in the banner it appears as version 0.6 (the actual version in the package __version__ attribute). I do not think that it is a problem, as in released versions both are almost always the same (except when "version_match" is "dev" or "stable", but I guess that in that case you want to display the number). Nevertheless, I mention it in case that it is not the intended behaviour.

@drammock
Copy link
Collaborator Author

I checked the banner by setting the version_match attribute manually to 0.1.

set it manually where? In conf.py?

In the dropdown it appears correctly as 0.1.

so you changed the versions.json file? Now I'm confused, version_match shouldn't show up in that file.

However in the banner it appears as version 0.6 (the actual version in the package __version__ attribute).

What the banner says is determined by DOCUMENTATION_OPTIONS.VERSION which is normally the package version. In contrast, version_match in conf.py is only used for styling the dropdown entry that matches version_match (i.e. it will highlight it)

@vnmabus
Copy link

vnmabus commented Sep 16, 2023

set it manually where? In conf.py?

Yes.

so you changed the versions.json file? Now I'm confused, version_match shouldn't show up in that file.

I didn't. It shows up as the default selected version, but if you expand the list of versions only the ones in "versions.json" appear. Sorry for the confusion.

What the banner says is determined by DOCUMENTATION_OPTIONS.VERSION which is normally the package version. In contrast, version_match in conf.py is only used for styling the dropdown entry that matches version_match (i.e. it will highlight it)

Ok, it was just to let you know.

@drammock
Copy link
Collaborator Author

t shows up as the default selected version, but if you expand the list of versions only the ones in "versions.json" appear.

Ah ok now I understand. That should not happen. Thanks for explaining.

@drammock
Copy link
Collaborator Author

It shows up as the default selected version, but if you expand the list of versions only the ones in "versions.json" appear.

I figured out why this happens. The default text for the button is here:

{{ theme_switcher.get('version_match') }} <!-- this text may get changed later by javascript -->

and it gets changed only if a "match" is found between what you set in conf.py and what you provide in the JSON file. In your conf.py you're using os.environ.get("RTD_VERSION")

https://github.com/vnmabus/dcor/blob/3baf2c253bd3f44b3f9f6e4644580b116685c8c7/docs/conf.py#L76-L79

which I'm guessing doesn't exist when you build locally, so no match is found, and thus the button text is never updated. In 37eb470 I've updated it to have a saner default text for the switcher button, which will make it easier to see if the match failed (yet still make sense and yield a functioning switcher)

@12rambau this one is ready for review.

After this I think we should push out a bugfix release, WDYT? Any other issues you want to tackle first?

@drammock drammock marked this pull request as ready for review September 18, 2023 16:32
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.

Thanks @drammock, nothing to say it works as expected and the wording works !

I tagged 3 other issues that I would like to solve before making a patch release.
If we don't solve them by Wednesday I'll release a patch anyway

PS: I opened an issue in sphinxcontrib.youtube, I'll work on it later: sphinx-contrib/youtube#63

@12rambau 12rambau merged commit 5db8dff into pydata:main Sep 18, 2023
14 of 18 checks passed
lagru added a commit to lagru/scikit-image that referenced this pull request Sep 22, 2023
0.14.1 fixes a broken version comparison in the javascript of the
version warning banner. See
pydata/pydata-sphinx-theme#1446 for more.
jarrodmillman pushed a commit to scikit-image/scikit-image that referenced this pull request Sep 22, 2023
* Enable version warning banners for docs

If the doc version isn't the preferred one, a warning banner is shown
at the top. This also accounts for dev builds.

* Update PyData theme to fixed 0.14.1

0.14.1 fixes a broken version comparison in the javascript of the
version warning banner. See
pydata/pydata-sphinx-theme#1446 for more.

* Revert to proper version_switcher.json URL
@drammock drammock deleted the fix-banner-version branch October 25, 2023 19:12
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.

Wrong version in warning banner
3 participants