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

Docs: mention that only semver versions are considered for the warning banner #8789

Closed
rhiaro opened this issue Dec 30, 2021 · 10 comments · Fixed by #10877
Closed

Docs: mention that only semver versions are considered for the warning banner #8789

rhiaro opened this issue Dec 30, 2021 · 10 comments · Fixed by #10877
Assignees
Labels
Accepted Accepted issue on our roadmap Needed: documentation Documentation is required

Comments

@rhiaro
Copy link

rhiaro commented Dec 30, 2021

We use a custom theme for the project on readthedocs. In Advanced Settings, the Show version warning setting is true. The docs say that a version warning banner for non-stable and non-latest versions will be inserted into a div with role="main". This was missing from the theme, so I added it, expecting the version warning to show up for this non-stable branch of the docs, but it hasn't. Is there something else I need to do to trigger the non-latest version warning?

@humitos
Copy link
Member

humitos commented Jan 3, 2022

Hi! I did a quick look on the page source you linked and I didn't find a div role="main" there. That's probably the problem.

@humitos humitos added the Needed: more information A reply from issue author is required label Jan 3, 2022
@rhiaro
Copy link
Author

rhiaro commented Jan 3, 2022

Thanks for checking. It's definitely there though.

Screenshot at 2022-01-03 12-07-43

@no-response no-response bot removed the Needed: more information A reply from issue author is required label Jan 3, 2022
@humitos
Copy link
Member

humitos commented Jan 3, 2022

Is that the version where you want to show the warning?

From the screenshot it seems it's rhiaro-theme-update version, which is not in semver format. So, I'm not sure it can be compared with the other ones to decide whether or not to show the banner.

I do see the request to https://standard.openownership.org/_/api/v2/footer_html/?callback=callback&project=beneficial-ownership-data-standard&version=rhiaro-theme-update&page=index&theme=sphinxtheme&format=jsonp&docroot=%2Fdocs%2F&source_suffix=.rst and it returns the 0.2.0 version, which seems to be the latest. However, the field is_highest is true for the current version, so the warning is not added (see

)

... and, if I understand correctly, as your version is not in semver format (as I guess in my first paragraph), we cannot compare it and we always mark it as is_highest=true. See

else:
ret_val['is_highest'] = True

I checked that chunk of code in production:

In []: from readthedocs.projects.version_handling import parse_version_failsafe
In []: v = p.versions.get(slug='rhiaro-theme-update')
In []: parse_version_failsafe(v.verbose_name)
In []:

IMO, the documentation should be updated to reflect that it only works on versions with semver format names. cc @astrojuanlu

@rhiaro
Copy link
Author

rhiaro commented Jan 3, 2022

Thanks, that's helpful! I had understood from the documentation that anything that wasn't in semver would not be classed as stable and therefore have a warning added, which is the behaviour I'd like. I had a go at hunting down the code in the rtd repo but couldn't find it, so I appreciate those links.

@humitos humitos added the Needed: documentation Documentation is required label Jan 4, 2022
@rhiaro
Copy link
Author

rhiaro commented Jan 6, 2022

Belated update - semver branch name solve the problem, thanks. I assume you're keeping this open for the docs update, but if not feel free to close.

Screenshot at 2022-01-06 13-16-28

@stsewd stsewd changed the title Show latest version warning for custom theme Docs: mention that only semver versions are considered for the warning banner Jan 6, 2022
@humitos humitos added the Accepted Accepted issue on our roadmap label Jan 27, 2022
@humitos
Copy link
Member

humitos commented Sep 13, 2023

We have deprecated the warning banner and it's currently only available for those projects that had it enabled already. We made this decision because all these confusions around when to show the banner and all the use cases involved here and each of them with their edge cases.

Lately, I've been thinking about "what's the best generic logic behind this problem?" that could cover most of the cases. I've reached to a simple implementation for the new addons we are working on on Read the Docs.

When a reader opens,

  • the latest version: a notification to warn the user about some features may not yet be available is shown, suggesting them to read the stable version instead.
  • a non-stable and not latest versions (e.g. v4.3 or v2.1rc2, etc): a notification to warn the user about reading a potential old/outdated version is shown and suggests to read stable version instead.

Would this logic works for your use case? I'd appreciate your feedback here 🙏🏼

In case you are curious, the PR that implements this logic is at readthedocs/addons#125.

@astrojuanlu
Copy link
Contributor

I think this logic would work perfectly for us 💯

@jertel
Copy link
Contributor

jertel commented Oct 27, 2023

We have deprecated the warning banner and it's currently only available for those projects that had it enabled already. We made this decision because all these confusions around when to show the banner and all the use cases involved here and each of them with their edge cases.

Lately, I've been thinking about "what's the best generic logic behind this problem?" that could cover most of the cases. I've reached to a simple implementation for the new addons we are working on on Read the Docs.

When a reader opens,

  • the latest version: a notification to warn the user about some features may not yet be available is shown, suggesting them to read the stable version instead.
  • a non-stable and not latest versions (e.g. v4.3 or v2.1rc2, etc): a notification to warn the user about reading a potential old/outdated version is shown and suggests to read stable version instead.

Would this logic works for your use case? I'd appreciate your feedback here 🙏🏼

In case you are curious, the PR that implements this logic is at readthedocs/addons#125.

The RTD docs may need to be updated to reflect this. I was trying to figure out why I couldn't find a setting on the Advanced Settings screen, and eventually stumbled onto this closed issue, which fortunately led me to this comment.

This is how the docs currently read:
image

And the actual page is here: https://docs.readthedocs.io/en/stable/versions.html#version-warning

I can take a stab at a PR to add a mention that this is now deprecated and only visible to older projects if you'd like.

@humitos
Copy link
Member

humitos commented Oct 30, 2023

I can take a stab at a PR to add a mention that this is now deprecated and only visible to older projects if you'd like.

Sounds great to me! Thanks!

Once readthedocs/addons#174 gets merged and deployed, we will re-enable the non latest/stable version warning banner addon for all projects. Maybe the documentation could adds a note mentioning the new addons as well if you consider.

@humitos humitos self-assigned this Nov 7, 2023
@humitos humitos moved this from Planned to Needs review in 📍Roadmap Nov 7, 2023
@humitos
Copy link
Member

humitos commented Nov 7, 2023

Once readthedocs/addons#174 gets merged and deployed, we will re-enable the non latest/stable version warning banner addon for all projects

This is done.

@github-project-automation github-project-automation bot moved this from Needs review to Done in 📍Roadmap Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Needed: documentation Documentation is required
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants
@humitos @astrojuanlu @rhiaro @jertel and others