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

feat(content-docs): draft docs excluded from build & sidebars #6457

Merged
merged 17 commits into from
Apr 13, 2022

Conversation

jodyheavener
Copy link
Contributor

@jodyheavener jodyheavener commented Jan 24, 2022

Motivation

The plugin-content-docs plugin does not currently provide a way for documents to be worked on while remaining hidden to visitors of the site. Its sibling plugin, plugin-content-blog provides this via the draft frontmatter property, which excludes the blog post from builds. This PR aims to reach parity with the blog plugin and introduce this functionality as well.

draft

Use case:

  • Document is actively being worked on, but is not ready for public consumption, and can be merged into the main/deployed Git branch without risk of leaking in production builds

Marking a document with draft: true...

  • Excludes it entirely from production builds
    • Remains visible in development
    • When built, accessing the URL yields a 404. There was discussion around allowing this to be customized such that instead of displaying a 404 you could display a "work in progress" message, however that behavior is inconsistent with the blog equivalent so I did not go that route. We could consider something like this in the future with an "unlisted" type draft status.
  • Excludes it from sidebars in production builds
    • This includes sidebars both manually and automatically generated
  • Excludes it from automatically generated index pages in production builds

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

First, create a new document with any content you wish. Add this document to a sidebar manually (or set up a sidebar to auto generate). This document should be visible in development and production builds.

To test draft...

  1. Add draft: true to the document's frontmatter, and save it.
  2. Start the development server. The expected result is this document remains accessible both directly and via its place in a sidebar (both manual and generated).
  3. Generate a production build and serve the static site. The expected result is this document is not accessible directly and is not visible in any sidebar (both manual and generated). Additionally, it should not show up in auto-generated index pages.

Related PRs

Closes #3417


🗺 I looked into how this impacts the sitemaps plugin, and for draft no extra work is necessary as these documents are already filtered out in routes.ts, preventing their page from being created and passing the route down to the plugin.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 24, 2022
@netlify
Copy link

netlify bot commented Jan 24, 2022

[V2]

Name Link
🔨 Latest commit 3b4d27e
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6256f0c186f1b40008f3d7a4
😎 Deploy Preview https://deploy-preview-6457--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Jan 24, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 47
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-6457--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Jan 24, 2022
@Josh-Cena Josh-Cena linked an issue Jan 30, 2022 that may be closed by this pull request
@slorber
Copy link
Collaborator

slorber commented Feb 3, 2022

Thanks @jodyheavener 👍 will review this asap but I'm busy trying to complete another pr

@jodyheavener jodyheavener force-pushed the jh/invisible-docs branch 2 times, most recently from 6f2a685 to e5a7e06 Compare February 4, 2022 19:01
@EFF
Copy link

EFF commented Feb 7, 2022

@slorber Any chances this could be shipped soon :) ?

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks, that looks nice 👍


Is having 2 booleans the best frontmatter model?


Is "unlisted" the best term? Maybe hidden or secret can also be nice?


Can you add tests to the _dogfooding folder please so that we can review this more easily?

https://docusaurus.io/tests/docs

  • autogen: normal doc + draft + unlisted
  • explicit sidebar: normal doc + draft + unlisted

I'd also like to have cases covering draft/unlisted on category link / index docs (explicit or autogen)

What if autogen + category/index.md has draft: true for example? We should probably "erase" the link from the sidebar category in this case (+ tests 😉 )


We want feature parity with docs and blog, and keep PRs small.

Maybe it is better to split this in 2: one PR for draft, one for unlisted?

Later we'll add another PR for unlisted on both blog + docs?

For "unlisted" we need to figure out sitemaps + noIndex + eventually find a way to remove links for navbar/footer.... there are more edge cases to consider and more code to write 😉 I think docs drafts can land sooner so it's worth it to split this

@jodyheavener jodyheavener force-pushed the jh/invisible-docs branch 2 times, most recently from 533b849 to d9039c2 Compare March 20, 2022 15:23
@jodyheavener jodyheavener force-pushed the jh/invisible-docs branch 2 times, most recently from c6b0996 to 06dce4c Compare March 30, 2022 23:18
@jodyheavener
Copy link
Contributor Author

jodyheavener commented Mar 30, 2022

@slorber Thanks for your review, and apologies for the delay in updates! I went ahead and removed the whole unlisted bit for now because you're totally correct - there are many more edge cases and additional requirements to address, and it'll be better to get the draft feature in first so it's not held up.

Also, I added a couple dogfooding tests (hope I did this correctly), one explicitly in the sidebar, and one through the auto-gen sidebar, just keep in mind that they will only be visible locally :)

@jodyheavener jodyheavener requested a review from slorber March 30, 2022 23:19
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

I'm not quite sure what the semantic of draft is. If we are to totally ignore it in production, I think we should just filter it out upfront and completely pretend it doesn't exist.

@jodyheavener
Copy link
Contributor Author

jodyheavener commented Apr 5, 2022

@Josh-Cena Thanks for the review! I renamed everything from isDraft to draft.

As for your point about filtering draft docs out earlier so we don't need to filter them out of the sidebar: I have spent some time playing with this and it seems like the problem is that you wouldn't be able to keep the doc referenced in a sidebar config while it's in draft. For example, in my dogfooding setup I have a draft doc that is manually defined in the sidebar, but when I build I get:

Error: Invalid sidebar file at "_dogfooding/docs-tests-sidebars.js".
These sidebar document ids do not exist:
- draft

Available document ids are:
...

Which makes sense since Docusaurus doesn't know about this doc anymore in production as it was filtered out. Do you see a way around this?

(My most recent push is going to fail because of this build failure - I will revert the commits if we decide not to go this route)

slorber added 3 commits April 8, 2022 17:01
# Conflicts:
#	packages/docusaurus-plugin-content-docs/src/__tests__/__snapshots__/index.test.ts.snap
@slorber
Copy link
Collaborator

slorber commented Apr 8, 2022

I'm not quite sure what the semantic of draft is. If we are to totally ignore it in production, I think we should just filter it out upfront and completely pretend it doesn't exist.

We have to keep the draft in the list so that we can keep the fail-fast behavior of the sidebar when a doc id typo is met => we don't want to filter all unknown docIds from the sidebar, but we want to filter only draft ids (so we need to know which docs exactly are drafts

Also knowing which docids are draft might be useful in other places like filtering navbar items

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

LGTM now 👍

Any opinion before I merge next week?

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Some random thoughts:

  • useLayoutVersion and useLayoutSidebar may return null as well, if the entire version / entire sidebar are drafts? What should be the UX here?
  • Need some more docs

@slorber
Copy link
Collaborator

slorber commented Apr 13, 2022

useLayoutVersion and useLayoutSidebar may return null as well, if the entire version / entire sidebar are drafts? What should be the UX here?

IMHO inferring a category full of drafts as a draft is already a quite fancy edge-case. If this becomes a feature that users clearly rely on, maybe we can have first class support to mark a category as draft, but current code is good enough?

Also if a category is full of drafts, then drafts will be removed, it will then be empty and empty categories are filered.

IE a sidebar full of draft is not filtered, but may end-up being entirely empty: {mySidebar: []} => technically this is quite similar to the absence of the same sidebar in terms of the final result.

Eventually we could filter the sidebar and also navbar items of type "docSidebar"? Not sure it's worth implementing today, but we can see if someone come up with a need for that, and maybe we'll allow adding a draft: true on the sidebar directly instead of adding draft: true to all sidebar docs frontmatter (ie, as soon there's an external link, sidebar couldn't be a draft anymore 😅 )

For versions, it's already possible to filter versions in the config file with onlyIncludeVersions option, that seems enough to me. Similarly, we could add filtering for "docsVersion" navbar item in this case? 🤷‍♂️


IMHO this PR is good enough in current state. Added the filtering to dogfood a bit but it's not 100% related to draft frontmatter.

Now if you want to add navbar items filtering for docSidebar + docsVersion in another PRs I'm fine with that 👍

@Josh-Cena
Copy link
Collaborator

I was mainly talking about the docSidebar and docVersion navbar items: should they be removed from the layout if the entire sidebar is a draft? But that sounds like an edge case, I need to take a look later. Didn't mean to block this.

# Conflicts:
#	packages/docusaurus-plugin-content-docs/src/plugin-content-docs.d.ts
@Josh-Cena Josh-Cena changed the title feat(content-docs): exclude draft docs from builds, sidebars feat(content-docs): exclude draft docs from build & sidebars Apr 13, 2022
@Josh-Cena Josh-Cena changed the title feat(content-docs): exclude draft docs from build & sidebars feat(content-docs): draft docs excluded from build & sidebars Apr 13, 2022
@slorber
Copy link
Collaborator

slorber commented Apr 13, 2022

👍 Seems good enough for now

Need some more docs

Agree, but the blog feature does not document that either outside the ref API so I did the same for consistency

We could also add a draft feature to md pages and maybe created a dedicated guide to describe better this feature that would work on all 3 content plugins? (later it could also document "unlisted")

Or do we add a section to each guide (or a small admonition tip?)

As this doc change spans across multiple content plugins, let's do this in another PR after we decide what's best

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Draft feature for Doc Pages Frontmatter
5 participants