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

Preserve relative path when switching from latest version of the documentation to an older version #22536

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

brianpursley
Copy link
Member

Fixes #22497

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 15, 2020
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Jul 15, 2020
@netlify
Copy link

netlify bot commented Jul 15, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 7bc5ee0

https://deploy-preview-22536--kubernetes-io-master-staging.netlify.app

@brianpursley
Copy link
Member Author

/assign @xiangpengzhao

@tengqm
Copy link
Contributor

tengqm commented Jul 15, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2020
@celestehorgan
Copy link
Contributor

/area web-development

I tested this by:

  1. Navigating to a docs page in 1.18, https://deploy-preview-22536--kubernetes-io-master-staging.netlify.app/docs/concepts/overview/components/, then switching to all available versions of the page.
  • Expected result: it works
  • Actual result: it works
  1. Navigating to a page I know doesn't exist before 1.18, https://kubernetes.io/docs/contribute/new-content/, then attempting to navigate to 1.17 and prior versions
  1. Navigating to a top-level page in 1.18, https://kubernetes.io/community/, and then switching to other available versions of the page
  • Expected result: it works
  • Actual result: it works
  1. Doing steps 1-3 in a translated version

Based on the results of #4 above, I compared this to the live site's existing behavior for old versions with untranslated content. Because the live site always redirects to the homepage from the version drop-down, this is a non-issue.


@brianpursley: Do you think there's any way we can handle the 404's found while testing and redirect them to the version's landing page? If the answer is "no" that's fine too, I think this is still a valuable contribution regardless. P.S., I think this would be a great contribution to upstream Docsy and you should totally do it!

@k8s-ci-robot k8s-ci-robot added the area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes label Jul 16, 2020
@celestehorgan
Copy link
Contributor

Related/probably closes: #16740

@brianpursley
Copy link
Member Author

brianpursley commented Jul 16, 2020

@celestehorgan Thanks for pointing that out... I see what you are saying. This change will definitely bring this problem to light. Even though it is not exactly a new problem, it would not occur currently, because it does not preserve the relative page when switching versions.

Let me take a look at it and I'll take a look at Docsy too and see what can be done about it.

@celestehorgan
Copy link
Contributor

@brianpursley If it's not easily solvable, then I'd rather post this issue to #sig-docs-maintainers and let the group decide whether or not to accept this change with the caveats found during testing. tl;dr don't spend too many cycles on this.

@brianpursley
Copy link
Member Author

@celestehorgan As you hinted at, it seems like this is going to be a little tricky.

The problem is that it is unknown if the relative path is going to 404 or not at the time when the version switch is made, only after it navigates can it be known whether that page exists or not (because each version is a different site altogether I believe).

I thought about one way it could work is the version switcher could add a querystring, something like ?version-switch=1 and then the 404.html could look for that querystring and redirect to the homepage, but that feels very hacky. Maybe there is a cleaner way to do that, but I don't know offhand.

I notice that 404.html only seems to be in English. I wonder if we could approach this problem from another angle and accept the fact that sometimes a page won't exist when switching versions, but implement language-specific 404 pages so that it is a better experience when it occurs.

Also, strangely, when I run the site locally, I don't get the custom 404 page at all, so something else must be handling the 404s when it is deployed. I admit, I'm not too familiar with the inner workings of the documentation website.

@sftim
Copy link
Contributor

sftim commented Jul 20, 2020

How about JavaScript that sends a HEAD request for the target version's page and, if it's a 404, lops off the path to take you to the top of the site instead?

(that's fine by me to add later, BTW)

@sftim sftim mentioned this pull request Jul 20, 2020
@sftim
Copy link
Contributor

sftim commented Jul 20, 2020

/lgtm

@sftim
Copy link
Contributor

sftim commented Jul 20, 2020

If we added some JavaScript to the 404 page that had access to Referer: header information, we could detect localization switches and customise the rendered content somehow (make a hidden <div> visible?)

@brianpursley
Copy link
Member Author

brianpursley commented Jul 20, 2020

How about JavaScript that sends a HEAD request for the target version's page and, if it's a 404, lops off the path to take you to the top of the site instead?

(that's fine by me to add later, BTW)

I like this idea and I was going to try something like below, but CORS blocks it when testing locally, and I think would also block it in production because each version is on a different subdomain

<a class="dropdown-item" href="javascript:selectVersion('{{ .url }}', '{{ $p.RelPermalink }}')">{{ .version }}</a>
...
<script type="text/javascript">
  function selectVersion(url, relPermalink) {
    $.ajax({
      type: "HEAD",
      async: true,
      url: url + relPermalink,
      complete: (xhr) => location.href = xhr.status === 404 ? url : url + relPermalink
    });
  }
</script>

I guess we could set CORS headers in netlify.toml to allow all origins (*) and allow the HEAD method. I haven't really worked with netlify before, but I think it would be something like this, and it would need to be applied to all versions of the site:

[[headers]]
  for = "/*"
    [headers.values]
    Access-Control-Allow-Origin = "*"
    Access-Control-Allow-Methods = "HEAD"

Any thoughts on setting CORS? Would that be OK or too much trouble?


EDIT: Alternatively, maybe your other idea about updating the 404 page will work for switching versions too.... detect a version switch and then redirect to the home page although the redirect might be a little annoying if it messes up the back button. I'll give that a try locally and see how it works though.

EDIT 2: Ah, maybe this would prevent the redirect from causing back button issues... document.location.replace(redirectURL)

@sftim
Copy link
Contributor

sftim commented Jul 20, 2020

Any thoughts on setting CORS? Would that be OK or too much trouble?

Allowing HEAD and in fact GET from everywhere seems fine to me. I'd like a second opinion before it gets a solid thumbs up, in case I'm missing something.

@brianpursley
Copy link
Member Author

Adding this script to the 404.html in layouts seems to work and would not require a cors change:

<script type="text/javascript">
  // If the referrer is a different version of the Kubernetes documentation,
  // don't show the 404 error page, but instead redirect to the root.
  var sameBaseUrlRegExp = new RegExp("^" + location.protocol + "//" + location.host.replace(".", "\.") + "/.*", "i");
  var kubernetesDocumentationUrlRegExp = new RegExp("^http[s]?://.*\.kubernetes\.io/.*", "i");
  if (!sameBaseUrlRegExp.test(document.referrer) && kubernetesDocumentationUrlRegExp.test(document.referrer)) {
    location.replace("/");
  }
</script>

If checks referrer against two regular expressions to figure out if you came from a different version of the documentation...

  1. Did you NOT come from the same base URL
  2. Did you come from a valid Kubernetes documentation URL... i.e. *.kubernetes.io?

On the plus side, this would not require a cors change

On the minus side, if 404.html is localized, it might need to be refactored to make it more reusable. Maybe hugo has a way to reuse this or we could put it in a js file I guess. Not sure what is best.

Anyway, I'm open to either approach either make the pre-navigation HEAD request to check, or update the 404 to detect the situation.

@sftim
Copy link
Contributor

sftim commented Jul 21, 2020

Maybe hugo has a way to reuse this

Yep! Partials or shortcodes. Probably a partial.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 21, 2020
@brianpursley
Copy link
Member Author

OK I just pushed a new version with a partial used in 404.html to redirect version switch 404s to the site root.

@kbhawkey
Copy link
Contributor

kbhawkey commented Aug 8, 2020

👀

@brianpursley brianpursley changed the title Preserve relative path when switching from version 1.18 of the documentation to an older version Preserve relative path when switching from latest version of the documentation to an older version Aug 17, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2020
@celestehorgan
Copy link
Contributor

celestehorgan commented Aug 24, 2020

I tested this by:

  1. Navigating to a documentation page in the most recent version, i.e. https://deploy-preview-22536--kubernetes-io-master-staging.netlify.app/docs/concepts/overview/working-with-objects/kubernetes-objects/
  2. Using the top menu version switcher to switch to an older version where I know the topic exists, i.e. 1.17

Expected/Actual results: The redirect works ✅

  1. Navigating to a non-documentation page in the most recent version, i.e. https://deploy-preview-22536--kubernetes-io-master-staging.netlify.app/partners/
  2. Using the top menu version switcher to switch to an older version where I know the page exists, i.e. 1.17

Expected/Actual results: The redirect works ✅

  1. Navigating to a documentation page in the most recent version, i.e. https://deploy-preview-22536--kubernetes-io-master-staging.netlify.app/docs/contribute/new-content/new-features/
  2. Using the top menu version switcher to switch to an older version where I know the topic DOESN'T exist, i.e. 1.17

Expected/Actual results: The 404 redirect works ✅

  1. Navigating to a non-documentation page in the most recent version, i.e. https://deploy-preview-22536--kubernetes-io-master-staging.netlify.app/training/
  2. Using the top menu version switcher to switch to an older version where I know the page exists, i.e. 1.16

Expected/Actual results: The 404 redirect works ✅

  1. Navigating to a known 404/bad url, i.e. https://deploy-preview-22536--kubernetes-io-master-staging.netlify.app/training_doesntexist/

Expected/Actual results: The page hits the 404, no regressions were introduced ✅


This PR behaves as expected and does not introduce any regressions. As such, it's approved, but with a hold because we're in repository freeze for the 1.19 release :)

/approve
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: celestehorgan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2020
@savitharaghunathan
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 26, 2020
@k8s-ci-robot k8s-ci-robot merged commit 390a4d8 into kubernetes:master Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Current page is lost when switching between documentation versions
8 participants