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(v2): include frontmatter in loadedContent doc metadatas #4495

Merged
merged 7 commits into from
Mar 26, 2021

Conversation

kaytwo
Copy link
Contributor

@kaytwo kaytwo commented Mar 23, 2021

Motivation

Fixes #4492

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Added a new test using the new config option. All previous tests still pass.

Related PRs

None right now, will add if this PR is accepted.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 23, 2021
@netlify
Copy link

netlify bot commented Mar 23, 2021

[V1] Deploy preview success

Built with commit 8b4cbdc

https://deploy-preview-4495--docusaurus-1.netlify.app

@netlify
Copy link

netlify bot commented Mar 23, 2021

Deploy preview for docusaurus-2 ready!

Built with commit 8b4cbdc

https://deploy-preview-4495--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Mar 23, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 83
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

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

@kaytwo kaytwo force-pushed the feat/frontmatter-in-globals branch from 647f8a3 to 3bc5541 Compare March 23, 2021 07:12
@slorber
Copy link
Collaborator

slorber commented Mar 24, 2021

going to close this for now as I don't think it's what we should do, cf #4492

@slorber slorber closed this Mar 24, 2021
@slorber slorber reopened this Mar 24, 2021
@slorber
Copy link
Collaborator

slorber commented Mar 24, 2021

oh actually this makes sense, it's just a "wording" issue.

We want frontmatter in loadedContent (ie available in plugin memory), but we don't want frontmatter in globalData

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.

I think we can add this, just need to remove the option and remove usage of global data.

It won't solve your sidebar problem directly but at least make the frontmatter available in loadedContent()

@@ -219,5 +219,6 @@ export function processDocMetadata({
)
: undefined,
sidebar_label: sidebarLabel,
frontMatter: options.includeFrontMatterInGlobals ? frontMatter : undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we want frontMatter here in all cases

@@ -12,6 +12,7 @@ export function toGlobalDataDoc(doc: DocMetadata): GlobalDoc {
id: doc.unversionedId,
path: doc.permalink,
sidebar: doc.sidebar,
...(doc.frontMatter && {frontMatter: doc.frontMatter}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't want frontmatter here, as it increases the size of all the site's pages

@@ -38,6 +38,7 @@ export const DEFAULT_OPTIONS: Omit<PluginOptions, 'id'> = {
versions: {},
editCurrentVersion: false,
editLocalizedFiles: false,
includeFrontMatterInGlobals: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove option?

@@ -54,6 +55,9 @@ export const OptionsSchema = Joi.object({
editUrl: Joi.alternatives().try(URISchema, Joi.function()),
editCurrentVersion: Joi.boolean().default(DEFAULT_OPTIONS.editCurrentVersion),
editLocalizedFiles: Joi.boolean().default(DEFAULT_OPTIONS.editLocalizedFiles),
includeFrontMatterInGlobals: Joi.boolean().default(
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

@@ -174,6 +181,7 @@ export type GlobalDoc = {
id: string;
path: string;
sidebar: string | undefined;
frontMatter?: FrontMatter;
Copy link
Collaborator

Choose a reason for hiding this comment

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

that seems wrong to me

@@ -142,6 +148,7 @@ export type DocMetadataBase = LastUpdateData & {
// eslint-disable-next-line camelcase
sidebar_label?: string;
editUrl?: string | null;
frontMatter?: FrontMatter;
Copy link
Collaborator

Choose a reason for hiding this comment

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

no ?

@slorber slorber changed the title feat: include frontmatter in globals feat(v2): include frontmatter in loadedContent doc metadatas Mar 26, 2021
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 👍

packages/docusaurus-plugin-content-docs/src/types.ts Outdated Show resolved Hide resolved
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.

Proposal: enable option to propagate arbitrary frontmatter data in globals
3 participants