-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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): last_update front matter #7461
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
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.
Implementation and tests look very solid for me. Well done! Just a few nits.
Waiting for @slorber to comment on the API design.
packages/docusaurus-plugin-content-docs/src/plugin-content-docs.d.ts
Outdated
Show resolved
Hide resolved
packages/docusaurus-plugin-content-docs/src/plugin-content-docs.d.ts
Outdated
Show resolved
Hide resolved
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.
Almost there, well done!
@@ -15,9 +15,9 @@ import { | |||
let showedGitRequirementError = false; | |||
let showedFileNotTrackedError = false; | |||
|
|||
export async function getFileLastUpdate( | |||
export function getFileLastUpdate( |
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.
keep this async, we'll likely make getFileCommitDate
async in the future
last_update: Joi.object({ | ||
author: Joi.string(), | ||
date: Joi.date().raw(), | ||
}), |
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.
do we allow empty last_update object? (no strong opinion, just curious to know what you think)
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.
Looks good overall 👍
Just small changes requested
packages/docusaurus-plugin-content-docs/src/__tests__/frontMatter.test.ts
Show resolved
Hide resolved
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.
LGTM thanks 👍
Pre-flight checklist
Motivation
This feature allows overriding Git history through front matter. It addresses part of #5691. It uses the API design mentioned here to be reused for created metadata and potentially a history.
Test Plan
Added unit tests for front matter configurations, including:
Created doc unit tests
Dogfooding
Test links
Deploy preview: https://deploy-preview-7461--docusaurus-2.netlify.app/
Dogfooding page: https://deploy-preview-7461--docusaurus-2.netlify.app/tests/docs/doc-with-last-update/
Documentation: https://deploy-preview-7461--docusaurus-2.netlify.app/docs/api/plugins/@docusaurus/plugin-content-docs/#markdown-front-matter
Related issues/PRs
#5691