Skip to content
This repository has been archived by the owner on Jan 8, 2025. It is now read-only.

Allow link hrefs with a scope query param #51

Merged

Conversation

ptgott
Copy link
Contributor

@ptgott ptgott commented May 9, 2022

Closes #35
Closes #33
See gravitational/teleport#11383

Previously, the Link component would always set the "scope" query
parameter in its href to the current value of DocsContext.scope.
I wanted to make it possible to link to other scopes, allowing readers
who are interested in another edition of Teleport to read content
intended for that edition.

We could add explicit links to different
scopes in, for example, warnings at the top of scope-irrelevant
pages or in introduction pages to sections that are only helpful for
a specific Teleport edition (e.g., the Enterprise section).

This change edits useNormalizedHref to add a "scope" value to the
href's query if the input href has a "scope" key and the value is
a valid scope name ("oss", "cloud", or "enterprise").

@vercel
Copy link

vercel bot commented May 9, 2022

@ptgott is attempting to deploy a commit to the gravitational Team on Vercel.

A member of the Team first needs to authorize it.

@ptgott
Copy link
Contributor Author

ptgott commented May 9, 2022

@iAdramelk Here's a PR based on my earlier discussion comment.

I thought it would be nice to add unit tests to useNormalizedHref, but ran over my allotted time figuring out how to configure the test suite. If I create a file called components/Link/hooks.test.tsx with this content:

import { suite } from "uvu";
import * as assert from "uvu/assert";
import { useNormalizedHref } from "./hooks";
const Suite = suite("components/hooks.tsx");

Suite("Preserves scopes in query strings", () => {
  const href = useNormalizedHref("/getting-started/linux-server/?scope=cloud");
  assert.equal(href, "/getting-started/linux-server/?scope=cloud");
});

Suite.run();

...I get this error from yarn test:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/paulgottschling/Documents/docs/node_modules/next/router' imported from /Users/paulgottschling/Documents/docs/components/Link/hooks.ts
Did you mean to import next/router.js?
    at new NodeError (node:internal/errors:372:5)
    at finalizeResolution (node:internal/modules/esm/resolve:405:11)
    at moduleResolve (node:internal/modules/esm/resolve:966:10)
    at defaultResolve (node:internal/modules/esm/resolve:1176:11)
    at resolve (file:///Users/paulgottschling/Documents/docs/node_modules/tsm/loader.mjs:1:551)
    at ESMLoader.resolve (node:internal/modules/esm/loader:605:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:318:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:80:40)
    at link (node:internal/modules/esm/module_job:78:36)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
error Command failed with exit code 1.

I've manually tested this change instead.

@vercel
Copy link

vercel bot commented May 9, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
docs ❌ Failed (Inspect) May 11, 2022 at 5:07AM (UTC)

Copy link
Contributor

@iAdramelk iAdramelk left a comment

Choose a reason for hiding this comment

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

I added some comments about already existing types that we can use to simplify code a little.

About the tests. Problem with them is in the next/router component, it works only inside the Next app. Calling it outside the Next app causes this error. We may emulate it for testing, but we will need to heavily mock it, so probably it would not be that useful. Alternatively we can rewrite this function in the way that receives router from the outside and test it this way, but it will make using the hook less convenient.

enterprise: true,
};

let scope: "oss" | "cloud" | "enterprise" = useContext(DocsContext).scope;
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: We already have types for scopes and the array of available scopes you can use here instead of defining them again.

See: https://github.com/gravitational/docs/blob/main/layouts/DocsPage/types.ts#L6-L9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed this in 182adf6

@ptgott ptgott force-pushed the paul.gottschling/2022-05-scoped-link-hrefs branch 2 times, most recently from 182adf6 to 73f7d10 Compare May 10, 2022 15:49
ptgott added a commit to gravitational/teleport that referenced this pull request May 10, 2022
Closes #11383

Note that this PR depends on merging a gravitational/docs PR:

gravitational/docs#51

Ensure that no visitor to the Teleport docs site sees content that is
irrelevant to their scope (e.g., Cloud, Open Source, or Enterprise) by
hiding scope-irrelevant content from the navigation menu.

In some cases, e.g., the introduction pages for the Cloud, Getting
Started, and Enterprise sections, the content is irrelevant to certain
scopes but a reader still may want to find out more.

One possibility would be to leave these section as-is for readers with
an unintend scope. However, these readers would see edition warnings in
the pages within a section unless they changed the value of the scope
picker.

As an alternative, this change used ScopedBlock to display links to
the relevant scope for users with a scope that is not intended for
a given menu page.

Also directs Enterprise visitors to the Getting Started menu page
to the Enterprise Getting Started page with an "enterprise" scope.
This fixes #10594.

Finally, wherever we include a warning at the top of an
edition-incompatible guide, I've added links to the scopes that
are supported for that guide. This should make it easier for
users to realize that they can adjust the scope to have the intended
docs experience.

// If a valid scope is provided via query parameter, adjust the
// link to navigate to that scope.
if (query.hasOwnProperty("scope") && scopes[query.scope] == true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can actually replace this code with scopeValues too(https://github.com/gravitational/docs/blob/main/layouts/DocsPage/types.ts#L6).

Something like scopeValues.includes(scope).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Made this change in 092e699

Closes gravitational#35
Closes gravitational#33
See gravitational/teleport#11383

Previously, the Link component would always set the "scope" query
parameter in its href to the current value of DocsContext.scope.
I wanted to make it possible to link to other scopes, allowing readers
who are interested in another edition of Teleport to read content
intended for that edition.

We could add explicit links to different
scopes in, for example, warnings at the top of scope-irrelevant
pages or in introduction pages to sections that are only helpful for
a specific Teleport edition (e.g., the Enterprise section).

This change edits useNormalizedHref to add a "scope" value to the
href's query if the input href has a "scope" key and the value is
a valid scope name ("oss", "cloud", or "enterprise").
@ptgott ptgott force-pushed the paul.gottschling/2022-05-scoped-link-hrefs branch from 73f7d10 to 092e699 Compare May 10, 2022 22:00
@ptgott
Copy link
Contributor Author

ptgott commented May 11, 2022

@iAdramelk Thanks!
@C-STYR Is there anything else we need to do before we can merge this?

@C-STYR
Copy link
Contributor

C-STYR commented May 11, 2022

@ptgott @iAdramelk the preview build is failing because it's OOM (exit code 137). @iAdramelk is the expert here, but sunsetting older versions of docs certainly can help. Now that @jeffgaynor is here and can be brought into the conversation, this may shift our priorities somewhat on how @Alqanar 's time is used.

@ptgott
Copy link
Contributor Author

ptgott commented May 11, 2022

@C-STYR @iAdramelk Thanks! Looks like there are intermittent OOMs with build jobs from other branches as well. I'll try the build again.

@iAdramelk
Copy link
Contributor

@ptgott @C-STYR

Yes, OOM errors right now it the side effect of the way build is organized now. The more pages we have and the larger they are, the more memory build uses. I'm working on a fix that can prevent it from happening in the future in this branch (#48), but it will take some time to finish. Meanwhile, sunsetting older versions can solve the problem for a time.

@ptgott
Copy link
Contributor Author

ptgott commented May 12, 2022

@C-STYR Looks like the Vercel build completed

@C-STYR C-STYR merged commit f5c2a45 into gravitational:main May 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants