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

Flyout: link to change version drops page name #211

Closed
3 tasks
agjohnson opened this issue Dec 9, 2023 · 19 comments · Fixed by #242 or readthedocs/readthedocs.org#11100
Closed
3 tasks

Flyout: link to change version drops page name #211

agjohnson opened this issue Dec 9, 2023 · 19 comments · Fixed by #242 or readthedocs/readthedocs.org#11100
Assignees
Labels
Accepted Accepted issue on our roadmap Bug A bug

Comments

@agjohnson
Copy link
Contributor

agjohnson commented Dec 9, 2023

Noticed here:

https://docs.astropy.org/en/stable/coordinates/inplace.html

The versions should all link to /en/{version}/coordinates/inplace.html. They link to /en/{version}/ instead.

  • Page name is dropped from version switch links
  • Page name is dropped from locale switch links (in theory?)
  • Latest version is used instead of current version for locale links
@agjohnson agjohnson added the Bug A bug label Dec 9, 2023
@agjohnson
Copy link
Contributor Author

Actually, to expand on this, I've noticed a couple issues with both of our flyout menus, so I'll note them all and we can work forward here.

Our old flyout also doesn't handle the locale links correctly after switching versions. That is:

  • A project with versions 1.0 and 2.0, set up on both locales en and es.
  • Go to /en/2.0, flyout links to alternate locales are /en/2.0 and /es/2.0.
  • Go to /en/1.0, flyout links to alternate locales are still /en/2.0 and /es/2.0. Links should instead be /en/1.0 and /es/1.0.

I'm also going to guess that the page name is dropped from the locale switcher links too.

@humitos
Copy link
Member

humitos commented Dec 11, 2023

The versions should all link to /en/{version}/coordinates/inplace.html. They link to /en/{version}/ instead.

I made this decision on purpose mainly because we can't guarantee that the page will exist in the other version (or language) and when clicking on these links the user will see a 404. So, it makes more sense to switch to the "homepage of the version/language"

There is more context in readthedocs/readthedocs.org#10102

I think both approaches have pros and cons and I decided to go with the simplest use case for now until we can have a better UX for users. If we want to implement "See this page in <language>" #130 (or "See the version <version> of this page" --which is essentially the same issue) we should somehow guarantee the page exists or don't show the link otherwise.

@humitos
Copy link
Member

humitos commented Dec 11, 2023

I made this decision on purpose mainly because we can't guarantee that the page will exist in the other version (or language) and when clicking on these links the user will see a 404. So, it makes more sense to switch to the "homepage of the version/language"

It seems that Material for MkDocs has arrived to the same conclusion: squidfunk/mkdocs-material#4835

The behavior described seems to be:

"Stay on the same page only if that same page exists. Otherwise, go to the homepage."

I'd like to see how the implemented UX is, because it could be pretty confusing as well if there is no indication that there is no translation/version for the page.

I think this opens the door to improve the UX on the front-end side without backend intervention maybe. The front-end could listen to the click even on those links and make a HEAD request to know if the page exists or not and decide what to do with that information:

  • redirect to homepage
  • go to the version/language of the current page
  • show a message saying there is no version/translation for that page and let the user decide what to do
  • etc

I'm not sure what's the best UX here yet, but I think this is probably a good direction to discuss more.

@humitos
Copy link
Member

humitos commented Dec 11, 2023

Explicit comment about this: squidfunk/mkdocs-material#4835 (comment)

@humitos humitos added the Needed: design decision A core team decision is required label Dec 11, 2023
@agjohnson
Copy link
Contributor Author

I made this decision on purpose mainly because we can't guarantee that the page will exist in the other version

Yeah, I figured that was the reason behind this. I think losing the page entirely is rather confusing UX, and users would encounter losing the page URL more commonly than hitting a missing page when switching versions. I think we optimize for the more common case here instead.

"Stay on the same page only if that same page exists. Otherwise, go to the homepage."

Yeah, this would be ideal UX.

I think we can easily do this from the backend. Proxito can redirect to the relative root if a missing page is encountered -- that is, we might link to /es/4.1/some/page.html?redirect-missing and proxito would know to automatically redirect instead of serving a 404.

Doing it from the front end is probably okay too, but I might be worried about edge cases like potential cross domain requests or handling existing redirects. I still think we should lean towards writing backend code when we can too.

If we want to implement "See this page in " #130 (or "See the version of this page" --which is essentially the same issue) we should somehow guarantee the page exists or don't show the link otherwise.

I think if we implemented anything like that, the UX should match the flyout. I'd focus on the UX at the flyout first before making any features that might conflict though. I'm not yet convinced we even need to implement a secondary form of language/version switching. The user/theme can always implement something like if needed.

An overlap here might be a user feature that warns when the page name doesn't match the referring page name -- "The page you requested does not exist so we're showing you the home page instead". Something to give the user explicit information about what just happened.

@agjohnson
Copy link
Contributor Author

Also, note #223 points out the same issue as here. I feel this is going to be a pretty expected UX.

@hugovk
Copy link

hugovk commented Jan 19, 2024

I would also prefer it to keep the page structure when changing versions.

For example, that's what the custom switcher currently used by https://docs.python.org. Here's some transitions when changing version and language:

  1. https://docs.python.org/3/tutorial/index.html
  2. https://docs.python.org/es/3/tutorial/index.html
  3. https://docs.python.org/es/3.10/tutorial/index.html
  4. https://docs.python.org/ko/3.10/tutorial/index.html
  5. https://docs.python.org/ko/3.12/tutorial/index.html
  6. https://docs.python.org/3.12/tutorial/index.html

If a page isn't found in the destination version, it shows the homepage:

  1. https://docs.python.org/3.13/howto/timerfd.html
  2. choose 3.12
  3. https://docs.python.org/3.12/

@agjohnson
Copy link
Contributor Author

It seems like the decision we need to make here is just which pattern we should use:

  • Proxito based redirect to the correct page if none exists
  • Javascript HEAD request check before following link/redirecting to the homepage

I lean towards a Proxito solution because I think we can be more precise about the redirect with application logic. The application can tell where to redirect the request to directly, where a JavaScript solution would need to do a series of guesses to come to the same conclusion. And we avoid any browser blocking and CORS side headaches.

@humitos do you have more opinions here?

@humitos
Copy link
Member

humitos commented Jan 22, 2024

I lean towards a Proxito solution because I think we can be more precise about the redirect with application logic. The application can tell where to redirect the request to directly, where a JavaScript solution would need to do a series of guesses to come to the same conclusion

Yeah, Proxito solution seems to be the most accurate, easier to maintain and gives us more control over time.

@humitos do you have more opinions here?

One problem that I see with this implementation is that we will loose the per-project cached flyout response and it will become per-page instead, degrading the performance of addons a lot. This is because the resolver will need to act over the filename of the current page instead: https://github.com/readthedocs/readthedocs.org/blob/3cfc29be8add5a0cf37d795e1c189d8aaa22a069/readthedocs/proxito/views/hosting.py#L350-L374

We may need a way to "resolve" the URL in the front-end, or communicate this to the backend somehow. Maybe we need to change the approach here and the URLs from the flyout need to be different:

  • https://docs.readthedocs.io/en/stable/intro/import-guide.html?readthedocs-language=es1 to tell Read the Docs that we want that particular page in Spanish. El Proxito will redirect it to:

    • same page name in Spanish (https://docs.readthedocs.io/es/stable/intro/import-guide.html) if it's not 404
    • home page in Spanish (https://docs.readthedocs.io/es/stable/), otherwise
  • https://docs.readthedocs.io/en/stable/intro/import-guide.html?readthedocs-version=latest to tell Read the Docs that we want that particular page in its latest version

    • same page name in latest version (https://docs.readthedocs.io/es/latest/intro/import-guide.html) if it's not 404
    • home page in latest version (https://docs.readthedocs.io/es/latest/), otherwise

Would this approach solve this issue without affecting our per-project cached flyout responses?

Footnotes

  1. if we don't want to interfere with real URLs here mixing "Read the Docs query attributes on user-based URLs" we can create an API endpoint for this: /api/v3/resolver/?url=https://docs.readthedocs.io/en/stable/intro/import-guide.html?language=es and /api/v3/resolver/?url=https://docs.readthedocs.io/en/stable/intro/import-guide.html?version=latest. This will probably be more clear for everybody and also without contaminating El Proxito code. Besides, this API endpoint could be re-used in other places where the front-end needs to resolve a URL in the same way the backend does.

@agjohnson
Copy link
Contributor Author

One problem that I see with this implementation is that we will loose the per-project cached flyout response

If we resolve the URL in the flyout API, I see this being a problem as well. I don't think we want to do this due to the cost of the resolver logic, even with caching.

Would this approach solve this issue without affecting our per-project cached flyout responses?

I think you're describing the pattern I was here. I was thinking we don't do anything different with the flyout response but would wait for the reader to click a link instead. That is, the flyout version/translation URLs would be naive and just simple text concatenation instead of using the resolver in the initial API response.

To avoid magic at Proxito we probably should have a dedicated endpoint, or should include some query parameter like what you're describing or simply https://docs.example.com/es/stable/maybe-this-404s.html?redirect=auto to force the Proxito view to find the most suitable URL.

@humitos
Copy link
Member

humitos commented Jan 23, 2024

I don't like too much the idea of using the same URL than a regular docs URL with ?readthedocs-*= attributes, but it seems the simplest to read and also the most explicit one. So, I'm on the fence between this approach and a dedicated API endpoint.

I'd avoid the implicit arguments like ?redirect=auto as you are describing, because that will be confusing and also could collide with user arguments. If we following the query arguments pattern on documentation URLs, I'd use ?readthedocs-*= explicit names.

IMO, the most technically accurate solution is a the API endpoint because that will give us lot of flexibility and re-usability as well. However, from that pattern I don't like the idea of using an API URL in the flyout as <a href="/_/api/v3/redirect/...">es</a>, so I would propose to keep the current URLs there (pointing to the home of each version) but capture the click event from those links and use front-end code to hit the API in the background:

document.querySelector("#spanish-translation").addEventListener("click", () => {
  const params = {
    "url": window.location.href,
    "language": "es",
  }
  fetch("/_/api/v3/redirect/", params, (data) => {
    window.location.href = data.redirect_to;
  });
});

I think this is a good compromise from our side:

  • 👍🏼 it doesn't contaminate Proxito's URLs
  • 👍🏼 it doesn't mix Read the Docs and user query parameters
  • 👍🏼 it uses a generic API endpoint that can be reused
  • 👎🏼 it always requires an extra request

@agjohnson What do you think?

I'm on the fence between this solution and using ?readthedocs-*= attributes on a regular documentation URL:

  • 👍🏼 it's easier to read and understand
  • 👍🏼 it doesn't require front-end code
  • 👍🏼 href on flyout is the real URL
  • 👎🏼 or 👍🏼? redirect logic and serving logic is in the same place (Proxito serving's code)
  • 👎🏼 not reusable code from the front-end

@agjohnson
Copy link
Contributor Author

If we following the query arguments pattern on documentation URLs, I'd use ?readthedocs-*= explicit names.

Yeah, prepending readthedocs is fine. I was only illustrating that a single query parameter could be used on the actual URL we are intending.

That is, the link URLs in the flyout would be:

  • https://docs.example.com/es/stable/some/page.html?readthedocs-redirect=auto
  • https://docs.example.com/en/1.2/some/page.html?readthedocs-redirect=auto

Instead of using the current version/language as the URL:

  • https://docs.example.com/en/stable/some/page.html?readthedocs-lang=es
  • https://docs.example.com/en/stable/some/page.html?readthedocs-version=1.2

Using the actual URL seems less confusing than URLs with multiple languages/versions -- once in the URL and once in the query params. The URL on hover would (almost all of the time) match the URL the user will end at after clicking the link.

IMO, the most technically accurate solution is a the API endpoint

I'm not so sure on this yet. If we aren't using the response from this request, then it feels like a mismatch for our API maybe.

We have needed in places to work around the resolver response time, usually relying on URLs returned from established API endpoints. Instead of using application code from the templates to resolve a URL to a version/page, UI elements capture point-in-time use of these links (on hover/click) and instead hit an API to get the resolver response. This is basically what you are describing with the second option.

However, I can't think of any place we still need generic access to a resolver API though. But it's worth noting, as if we did need this still, there would be multiple uses for a generic API like this.

@humitos
Copy link
Member

humitos commented Jan 24, 2024

That is, the link URLs in the flyout would be:
* https://docs.example.com/es/stable/some/page.html?readthedocs-redirect=auto
* https://docs.example.com/en/1.2/some/page.html?readthedocs-redirect=auto

OK, I think you convinced me about this 😄 . I will add this issue to the roadmap and the next sprint.

@humitos humitos added Accepted Accepted issue on our roadmap and removed Needed: design decision A core team decision is required labels Jan 24, 2024
@humitos humitos self-assigned this Jan 24, 2024
@humitos
Copy link
Member

humitos commented Jan 24, 2024

@agjohnson what do you think about the following issues? Can you comment there? I think we can tackle the three of them at the same time:

@agjohnson
Copy link
Contributor Author

The anchor in #131 would be a new feature of both flyouts, but would be nice to have eventually. I see it not being a huge feature on top of parsing and building link URLs though, so it might be worth giving a quick try at. I'd say use your judgement there, bail out if it feels like too much work.

#130 seems like what I noticed above too, and would be a good issue to address at the same time.

OK, I think you convinced me about this 😄

I will say that my suggestion is still just a guess, and I don't dislike the other solutions we've discussed here so far. I might not being considering the API response as deeply as I should either.

I'm assuming we won't have trouble building URLs in JS with data from the API response. Thinking a little more, I could see generating version/language specific URLs in the response might require a per-version/language cached API response instead of per-project -- but at least that is far better than per-page.

@ericholscher
Copy link
Member

ericholscher commented Feb 7, 2024

Ideas for implementation here:

  • Pass the filename data into HTML via the Cloudflare worker via HTTP header from proxito, and use that data in the addons JS via the HTML.
  • Return a string in the Addons API endpoint that we can use to get the filename from the current document URL (eg. document.url.replace('/en/latest/'))

We should add a GET argument onto the URL (?from_footer=True) so that we can track what percentage return 200 or 404, and move forward on what to do from there. If users want to redirect those to the front page, they can do that themselves (Page redirect: *?from_footer=True -> /)

We could pass the data in the file similarly to the Sphinx extension, which currently outputs this during Sphinx builds:



<script type="application/json" id="READTHEDOCS_DATA">{"ad_free": false, "api_host": "https://readthedocs.org", "builder": "sphinx", "canonical_url": null, "docroot": "/docs/user/", "features": {"docsearch_disabled": false}, "global_analytics_code": "UA-17997319-1", "language": "en", "page": "index", "programming_language": "py", "project": "docs", "proxied_api_host": "/_", "source_suffix": ".rst", "subprojects": {}, "theme": "sphinx_rtd_theme", "user_analytics_code": "UA-17997319-6", "version": "stable"}</script>

<!--
Using this variable directly instead of using `JSON.parse` is deprecated.
The READTHEDOCS_DATA global variable will be removed in the future.
-->
<script type="text/javascript">
READTHEDOCS_DATA = JSON.parse(document.getElementById('READTHEDOCS_DATA').innerHTML);
</script>

humitos added a commit to readthedocs/readthedocs.org that referenced this issue Feb 8, 2024
Return `X-RTD-Resolver-Filename` from El Proxito and inject it as HTML `meta`
tag from Cloudflare Worker.

Required by readthedocs/addons#211
humitos added a commit to readthedocs/readthedocs.org that referenced this issue Feb 8, 2024
Return `X-RTD-Resolver-Filename` from El Proxito and inject it as HTML `meta`
tag from Cloudflare Worker.

Required by readthedocs/addons#211
humitos added a commit to readthedocs/readthedocs.org that referenced this issue Feb 27, 2024
…11100)

* Development: use `wrangler` locally (update NGINX/Dockerfile config)

Related readthedocs/addons#217

* Add CF Worker .js script

* Proxy JS files to Addons GitHub's repository

* Point common to wrangler branch

* Addons + Proxito: return `X-RTD-Resolver-Filename` and inject via CF

Return `X-RTD-Resolver-Filename` from El Proxito and inject it as HTML `meta`
tag from Cloudflare Worker.

Required by readthedocs/addons#211

* Feedback from review

* Validate the header value before injecting it

* Initialize `request.unresolved_url` as `None`

* Add test for longer filename

* Inject the `unresolved_url` on 404 pages
@agjohnson
Copy link
Contributor Author

Backend changes are good, #242 is close, reopening this to track that.

@agjohnson agjohnson reopened this Feb 28, 2024
humitos added a commit that referenced this issue Mar 5, 2024
humitos added a commit to readthedocs/common that referenced this issue Mar 5, 2024
We lost this work when moving the `.js` file from the ops repository into
community and then again to common.

This is required to be deployed together with addons 0.10.0.

Related:

* readthedocs/readthedocs.org#11126
* readthedocs/addons#211
@humitos
Copy link
Member

humitos commented Mar 5, 2024

This change is deployed. Now, switching between versions/languages from the flyout addons will keep the page the user is looking at on the version/language destination.

humitos added a commit to readthedocs/common that referenced this issue Mar 5, 2024
We lost this work when moving the `.js` file from the ops repository into
community and then again to common.

This is required to be deployed together with addons 0.10.0.

Related:

* readthedocs/readthedocs.org#11126
* readthedocs/addons#211
@astrojuanlu
Copy link

Tested it and it works 🙏 Thank you folks!

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 Bug A bug
Projects
Archived in project
5 participants