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
Added policy pages #20
Conversation
👋 I'm your friendly documentation robot, and I'm going to build your pull request on our staging site so that you can review it. If you push up subsequent commits, I'll rebuild your staging area (and when this pull request is closed, I'll clean that staging area up). I'll let you know when I'm done! 🤖 |
👋 I've finished building this pull request on our staging site. 🤖 To see how it looks, visit: https://docs-staging.npmjs.com/20/ |
There are couple of links that are missing to be updated from the cli-v6 and cli-v7 before this PR can be merged |
updates to cli/v6 and cli/v7 documentation has been done respectively in PRs npm/cli#3522 and npm/cli#3523. |
so this is moving policies from the npm site to the docs site? is there any way we can get a redirect put in place so the old urls will still work? even if we merge the updated urls into the cli, our user base tends to be pretty slow moving to install even patch updates, so the majority of folks would still be getting the old urls. a redirect would sure help smooth this out! |
1cbab25
to
40643bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some notes about the vendored CLI docs needing to be fixed upstream in the CLI repo. Those changes likely shouldn't be made in this PR but updated upstream and in turn vendored in through automation.
I noticed the pacakge-lock.json has been deleted. That file should be restored.
I'm going to double check locally that the policy pages are unchanged
@MylesBorins I have forgotten to remove the updated files for the CLI. As mentioned above
Will include the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More comments inline. One thing I noticed that is broken right now are links and how things are currently organized.
The main content is available at policies... but if you click through to view terms it is available at /terms
rather than /policies/terms
. The links within these pages have absolute URLs pointing to docs.npmjs.com, meaning there is not way to click through and verify the links in a local dev environment. This does not appear to be the case with other self referencing links elsewhere in the docs site such as the link to "npm organizations" found in about npm
We need to get these issues fixed before we can ship this.
You can programmatically update the We already omit the CLI from those - for backcompat. But here's a diff that will omit policies as well, which is indeed what we want. diff --git a/gatsby-node.js b/gatsby-node.js
index f397ae91..91bba05a 100644
--- a/gatsby-node.js
+++ b/gatsby-node.js
@@ -1,11 +1,22 @@
exports.onCreateNode = ({node, actions, getNode}) => {
const {createNodeField} = actions;
+ // for backward compatibility with the existing documentation site,
+ // individual pages (but not directory indexes) use their name as the
+ // entire slug for the url.
+ //
+ // For example: `enterprise/sunset/sunsetting-npm-enterprise.mdx` becomes
+ // just `https://docs.npmjs.com/sunsetting-npm-enterprise` (omitting the
+ // directory structure).
+ //
+ // The exceptions to this are the CLI documentation (again, for historical
+ // purposes) and the policies (for an improved UX)
if (node.internal.type === "Mdx") {
const file = getNode(node.parent);
- // cli paths are unchanged
- if (file.relativeDirectory.startsWith('cli/')) {
+ // cli and policies paths (and their subdirectories) are unchanged
+ if (file.relativeDirectory.match('^cli(\/.*)?$') ||
+ file.relativeDirectory.match('^policies(\/.*)?$')) {
return;
}
(You'll need to update the nav as well once you land this diff.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM with some small nits.
FWIW these things can also be fixed after landing
Co-authored-by: Myles Borins <mylesborins@github.com>
…add-policy-pages * 'add-policy-pages' of github.com:npm/documentation: Update content/policies/index.mdx
npm policy pages are available from npm Docs