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

NEW: Add version warning banners #780

Closed
wants to merge 5 commits into from

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Jul 1, 2022

This adds functionality in our version switcher logic to add a warning banner directing users to a different version if they are not on a specific version of the documentation.

It uses a placeholder ID in our theme template that comes just after the header navigation bar, and adds some JavaScript to the version switcher logic that looks for this placeholder and appends the warning banner just after

chrome_HQiVlpIPPK

I'm not really sure how to test this one, since basically all the functionality is in JavaScript...

closes #759

@choldgraf

This comment was marked as resolved.

@bryevdv
Copy link
Contributor

bryevdv commented Jul 1, 2022

@choldgraf This looks nice, a couple of comments:

  • I think the specificity of distinct messages for "outdated" vs "unreleased" is a nice-to-have, but a combined single message will do if it's much simpler on your end

  • Having to specify an explicit version in direct_to_version seems like it might be limiting? What I really want is just to always go to the highest, non-dev version in the switcher, always. We deploy our docs to a CDN so when we publish a new "latest" version, we push two copies: one to /en/<version> and another identical copy to (replace) /en/latest. I just want to make sure that the banner does not show up for both of those.

I'm not really sure how to test this one, since basically all the functionality is in JavaScript...

Selenium is probably your best bet

@choldgraf
Copy link
Collaborator Author

Would it be safe to make these assumptions?

  • The version (in switcher.json) is always called "dev" or "development"
  • the version we direct users to is always called "stable"
  • All other versions are for previous releases

If so, then I don't think the logic you mention would be too tough. What I want to avoid is lots of new config surface area because every variable can lots of different spellings etc. But I'm not sure whether all of these assumptions hold for all of the stakeholder projects we care about 🤔

@bryevdv
Copy link
Contributor

bryevdv commented Jul 1, 2022

The version (in switcher.json) is always called "dev" or "development"

I might be misunderstanding of some aspect of how the switcher works. I don't really understand what this is asking. If this is about what dev version strings look like, the can have either "dev" or "rc". I imagine it is harder to reliably discern pre-releases in JS, without packaging.version, so like I said, a combined message for both outdated/unreleased seems fine.

the version we direct users to is always called "stable"

It doesn't matter to me what terminology is used in configuration, but we have used /en/latest for the URL location for ~10 years. I would be happy to unconditionally be able to say "always direct old/outdated pages to /en/latest.

I guess I should try to understand better how the PST switcher functions, and how a page can determine if it is "not stable". How do PST pages "know" what version they are for? Do they pull it out of the page's URL? Or is there a const window._VERSION = <...> somewhere that gets inserted? For us currently, we do the latter. So let's say we push Bokeh 2.4.4 then both the pages at /en/2.4.4 and /en/latest will know they are for "2.4.4" and this is the version that is compared against "latest" in our versions.json to suppress the warning banner. Or put another way, "latest" (or "stable") is never an actual version it is more like a pointer to a version. I guess I am surprised that switcher.json does not have a field like "stable": <some specific real version>

@dopplershift
Copy link

On MetPy we haven't yet switched to the built-in version selector (we're on an old one). We check against DOCUMENTATION_OPTIONS.VERSION and point to latest (which is a symlink) if that version isn't the latest, as defined in the json file in the root of the doc repo.

@bryevdv
Copy link
Contributor

bryevdv commented Jul 1, 2022

On MetPy we haven't yet switched to the built-in version selector (we're on an old one). We check against DOCUMENTATION_OPTIONS.VERSION and point to latest (which is a symlink) if that version isn't the latest, as defined in the json file in the root of the doc repo.

That sounds exactly how we do it with Bokeh except I was not aware of DOCUMENTATION_OPTIONS.VERSION I don't see how to escape a need for "current stable/lastest" to be explicitly specified in the top-level json file.

@choldgraf
Copy link
Collaborator Author

Hmmm - what about a system like:

  • In version.json one of the entries can have a preferred: true key/val
  • If the version of the docs does not match this entry, then:
    • If the name has dev in it, then assume the current version is unreleased and point to the preferred version.
    • If the name doesn't have dev in it, assume it's an old, released version, and point to the preferred version.
    • Build a banner with the proper language according to the assumptions above

@bryevdv
Copy link
Contributor

bryevdv commented Jul 1, 2022

In version.json one of the entries can have a preferred: true key/val

This would work, but it also seems somewhat more difficult to maintain. Updating now requires finding the old preferred and deleting it, and then adding preferred to the new block. And what happens if two or more blocks accidentally specify preferred at the same time? I guess that might be the only option without changing the top-level structure of switcher.json, though, since it is just a list :/

Re: oudated vs unreleased, I should have not mentioned it :) I think it's best to not worry about it and just have a combined banner message with link (as I mentioned we have "rc" pre-releases in addition to "dev"... absent a reliable way to determine pre-releases it seems to fragile to worry about)

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jul 1, 2022

yeah - I guess the other approach we could take is to let the switcher.json file have two top-level keys:

  • config: "key-val pairs that control various behavior, such as 'prefer_version'"
  • versions: our current "list"

and to avoid a breaking change the logic could be "if switcher.json is a dictionary, assume it has keys config and versions, and if it is a list, assume it is just a single list reflecting versions". I can't think of any other config now, but I could imagine a generic config key being useful for other stuff in the future? e.g. config.direct_message which could control the message that's displayed if somebody wanted something custom 🤷

@bryevdv
Copy link
Contributor

bryevdv commented Jul 1, 2022

@choldgraf I personally think that would be a good (forward-looking) change but I'm also only thinking about the benefits, not any costs :) Obviously that's something for the PST team to weigh

@choldgraf
Copy link
Collaborator Author

I mean, I think that in general having the top-level structure of the JSON file be a dict instead of a list will make it more extensible/future-proof. Maybe at the cost that it will be more of a pain to write? But then again it's only a one-time file that you just periodically update...

@dopplershift
Copy link

@choldgraf I'll just state that we're generating our json file with every major doc release automatically: https://github.com/Unidata/MetPy/blob/gh-pages/versions.json

@choldgraf
Copy link
Collaborator Author

Could you (or others) share your opinion on what seems like an intuitive and hassle free config structure to trigger other kinds of behavior like "direct people to version X"? Does my proposal above seem to make sense?

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jul 2, 2022

Hmmm, thinking about this more:

I think that the simplest path forward would be to leave the configuration as, is, but introduce two optional keys per entry:

  • "preferred": "true". If found, this entry is assumed to be the one that all other versions should direct to. We assume any entry without this key is for a development version or a previous release.
  • "description": "some description": If found, this would be used as a description of the version, instead of "a previous release".

Then the logic would be:

  • If switcher.json has an entry with preferred: true, then store it as PREFERRED_RELEASE and do the following
  • Check if the current page's version matches preferred: true. If so, do nothing. If not then:
  • Create a banner with a link to the preferred version, and with the message: This documentation is for a {DESCRIPTION}. Switch to {PREFERRED RELEASE}.

So as a maintainer, you would need to:

  • Mark your "preferred" release with "preferred": "true".
  • Add a description to any you wanted a custom description for.
  • Depending on your naming, you might have to update the entry with "preferred": "true" when you make a new release.

At least this way the only configuration changes are in the one switcher.json file, so even if the configuration needs to change in the future, it is just a one-time change (AKA you don't have to dig back through old versions of your docs and change stuff)

I just pushed a commit that shows off this behavior, and updates the docs

@bryevdv
Copy link
Contributor

bryevdv commented Jul 5, 2022

Mark your "preferred" release with "preferred": "true".
Add a description to any you wanted a custom description for.
Depending on your naming, you might have to update the entry with "preferred": "true" when you make a new release.

Just speaking for myself, this would increase the amount of manual work past the value of shedding our template customizations, and I would just keep the system we have in place presently.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jul 5, 2022

@bryevdv I'm a bit confused - with the current system, you have to manually add new versions to the JSON when a new version is released. It doesn't seem like this is any more work than what you'd already have to do. Maybe you are using the versions.json file in a different way from this one? E.g., if we were to bump the versions.json to a new release, we'd just change the file from this:

  {
    "name": "dev",
    "version": "latest",
    "description": "the development version",
    "url": "https://pydata-sphinx-theme.readthedocs.io/en/latest/"
  },
  {
    "name": "0.9.0 (stable)",
    "version": "stable",
    "preferred": "true",
    "url": "https://pydata-sphinx-theme.readthedocs.io/en/stable/"
  },
  {
    "name": "0.8.1",
    "version": "v0.8.1",
    "url": "https://pydata-sphinx-theme.readthedocs.io/en/v0.8.1/"
  },

to this:

  {
    "name": "dev",
    "version": "latest",
    "description": "the development version",
    "url": "https://pydata-sphinx-theme.readthedocs.io/en/latest/"
  },
  {
    "name": "0.10.0 (stable)",
    "version": "stable",
    "preferred": "true",
    "url": "https://pydata-sphinx-theme.readthedocs.io/en/stable/"
  },
  {
    "name": "0.9.0 (stable)",
    "version": "stable",
    "url": "https://pydata-sphinx-theme.readthedocs.io/en/v0.9.0/"
  },
  {
    "name": "0.8.1",
    "version": "v0.8.1",
    "url": "https://pydata-sphinx-theme.readthedocs.io/en/v0.8.1/"
  },

That said, if folks in general think it is too much manual work/toil to change 1 line of the JSON file each time there's a release, then I don't think we'll be able to incorporate this feature in a project-agnostic way. Do others agree this would be too much work? If everybody is just going to continue maintaining their own custom templates we can just close this one.

@bryevdv
Copy link
Contributor

bryevdv commented Jul 5, 2022

change 1 line of the JSON file each time there's a release

The issue for me is that rather than simply adding or replacing a line (even a block), which is something I might consider automating, I'd have to find the old block in which a previous release was marked preferred, and edit/delete it to remove the preferred flag as well. This seems fragile since it seems like it would be trivially easy to accidentally mark or leave two or three release blocks as preferred unintentionally, resulting (I assume) in undefined behavior. In order to prevent that, we'd have to add some kind of specialized linter to check that only a single block is marked "preferred", so now we are developing new tooling as well. This is why I think having a single explicit field like "latest": "4.3.7" in one place outside the list of versions is what is needed. When I created the issue, I did (mistakenly) assume that was already present (or could be).

we can just close this one.

Given that projects do seem to have their own unique requirements around versions, perhaps a more approachable goal would be to simply offer a consistent mechanism to add/populate a warning banner below the menu from JS (with whatever content is appropriate for their project).

@choldgraf
Copy link
Collaborator Author

This seems fragile since it seems like it would be trivially easy to accidentally mark or leave two or three release blocks as preferred unintentionally

I wonder if you are using a different naming strategy than this theme uses. In the example I gave above, the "preferred" version is always the entry named stable. So the preferred: true entry never moves, and we have to update the name entry anyway because it's changing to be a new version (the same would be true of latest as well, if that was our preferred version).

So for example, here's a GIF of the changes that would need to be made when we released a new version, note that preferred: true is never moving:

Code_ZKTrrFgENi

@bryevdv
Copy link
Contributor

bryevdv commented Jul 5, 2022

@choldgraf I do think we have some terminology differences. But I was able to distill the important difference to this I think: The theme seems to assume that the "latest" version does not also get its own "x.y.z" link.

But for us, if Bokeh 4.3.7 is the latest release, then both of these links will work, and both should not have the warning banner:

  • https://docs.bokeh.org/en/4.3.7/
  • https://docs.bokeh.org/en/latest/

And I just don't see how that is feasible unless the page can compare its actual package version. I.e. compare "4.3.7" which is the value of DOCUMENTATION_OPTIONS.VERSION for both pages (and not "latest" since "latest" is not a real version) to some fixed, unambiguous declaration in the remote JSON like "latest": "4.3.7"

It's clear that I thought this was going to be a simple ask, but it's also clear that it has not turned out to be. :) It's definitely fine with me if you want to close this and the issue, because I did intend for things to spin out to so much work/discussion.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jul 5, 2022

Hmmm ok. So just to confirm, you think that this problem would go away if the configuration could be something like:

{
"preferred": "<some-version",
"versions": [<list-of-versions>],
}

Where <list-of-versions> is the current contents of switcher.json

@bryevdv
Copy link
Contributor

bryevdv commented Jul 5, 2022

@choldgraf if we have this:

{
"preferred": "4.3.7",
"versions": [<list-of-versions>],
}

then assuming:

  • Any page (regardless of URL location) with DOCUMENTATION_OPTIONS.VERSION == "4.3.7" gets no banner, and
  • On all other pages, the button on the banner can be made to link to /en/latest all the time

then yes.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jul 5, 2022

Well in our case we'd check for the version_match variable that's defined in conf.py and I think the logic would be "is this documentation's version_match == "preferred": "<version>"? If not, then add a banner with a link to the URL that corresponds to "preferred": "<version>".

I opened an issue about changing the configuration structure to see if others had thoughts pro/con here:

@bryevdv
Copy link
Contributor

bryevdv commented Jul 5, 2022

Well in our case we'd check for the version_match variable that's defined in conf.py and I think the logic would be "is this documentation's version_match == "preferred": ""

I guess this value gets injected into the rendered page analogous to DOCUMENTATION_OPTIONS.VERSION so that the comparison happens in JS? Then 👍 that should work too.

@choldgraf
Copy link
Collaborator Author

Yeah - though now I'm wondering if we should just use DOCUMENTATION_OPTIONS instead. How does that get populated?

@bryevdv
Copy link
Contributor

bryevdv commented Jul 5, 2022

Yeah - though now I'm wondering if we should just use DOCUMENTATION_OPTIONS instead. How does that get populated?

I believe it is specifically the value of release (not version) in conf.py

Yes: https://github.com/sphinx-doc/sphinx/blob/95b81831b19727a3a933e387b1bc6c1053c3aadd/sphinx/themes/basic/static/documentation_options.js_t

@choldgraf choldgraf added this to the 0.11 milestone Jul 6, 2022
@jarrodmillman jarrodmillman modified the milestones: 0.11, 0.12 Oct 4, 2022
@12rambau 12rambau modified the milestones: 0.12, 0.13 Nov 17, 2022
@choldgraf choldgraf removed this from the 0.13 milestone Jan 9, 2023
melissawm added a commit to melissawm/napari-sphinx-theme that referenced this pull request May 2, 2023
Adds a a warning banner to be shown on non-stable versions of the docs.
Inspired by pydata/pydata-sphinx-theme#780
@12rambau
Copy link
Collaborator

12rambau commented Jun 8, 2023

superceeded by #1335

@drammock drammock mentioned this pull request Jul 18, 2023
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.

Support for loud version warning banners
5 participants