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): new "created" metadata #7588

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

dpang314
Copy link
Contributor

@dpang314 dpang314 commented Jun 9, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

This feature allows displaying the date that documentation was created. It uses Git history, but can be overridden through front matter. It addresses the second half of #5691.

Sometimes the wording is a bit awkward, since last update is a noun, but create isn't, but I used create/created for consistency. creation fits better grammatically in a few spots, but I don't know if that would be preferable if it breaks consistency.

Test Plan

Added unit tests for front matter configurations, including:

  • create front matter undefined
  • Author undefined but date valid
  • Author valid but date undefined
  • Invalid date strings
  • Both valid with varying date string formats

Created doc unit tests

  • Front matter overrides Git history
  • Front matter with only author uses Git history for date
  • Front matter with only date uses Git history for author
  • Front matter is not shown when config disables it

Dogfooding

  • Created new page with create set with front matter
  • Create time/author generated by Git history can be seen on the other dogfooding pages

Test links

Deploy preview: https://deploy-preview-7588--docusaurus-2.netlify.app/
Dogfooding page: https://deploy-preview-7588--docusaurus-2.netlify.app/tests/docs/doc-with-create/
Dogfooding page with create from Git history: https://deploy-preview-7588--docusaurus-2.netlify.app/tests/docs/
Documentation: https://deploy-preview-7588--docusaurus-2.netlify.app/docs/api/plugins/@docusaurus/plugin-content-docs/

Related issues/PRs

#5691

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 9, 2022
@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Jun 9, 2022
@Josh-Cena Josh-Cena changed the title feat(content-docs): created-metadata feat(content-docs): new "created" metadata Jun 9, 2022
@netlify
Copy link

netlify bot commented Jun 9, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 99ded9d
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62bd69fa3bedbb00090c1bc2
😎 Deploy Preview https://deploy-preview-7588--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 Jun 9, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟢 90 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 82 🟢 99 🟢 100 🟢 100 🟢 90 Report

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.

Implementation is nice, tests are first-class, but a lot of duplicated code that has best be cleaned up before progressing. I also can't remember my motivation for this feature in the first place.

let showedGitRequirementError = false;
let showedFileNotTrackedError = false;

export async function getFileCreate(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's merge this file with getFileLastUpdate? There's a lot of duplicated code here. getFileCreate is also not a nice name; maybe getFileCreateInfo... 🤔

Comment on lines 58 to 59
| `showCreateAuthor` | `boolean` | `false` | Whether to display the author who created the doc. |
| `showCreateTime` | `boolean` | `false` | Whether to display the date the doc was created. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's been close to a year and I don't even wholly recall my motivation for this feature...😅 Do we want it as an out-of-the-box feature, or only provide the metadata and wait for users who swizzled it to discover it? (Not a nice thing to do) I don't have a compelling use-case apart from telling two sets of documentation apart (which one is "legacy" and which one is "current"). We could use also use versioning for that purpose, though. I would need to think of a better use-case. Eventually we need to address #6218, which requires much more metadata than this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can’t think of a very good use-case either. I thought there was a reason that I didn’t think of 😅. Should I continue working on this feature? I am happy to close the pull request if there is no strong reason for keeping it.

Copy link
Collaborator

@slorber slorber Jun 22, 2022

Choose a reason for hiding this comment

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

Given it's not too complex, opt-in, and may give additional flexibility by exposing the creation date, I think we can keep working on this.

The other possibility for users would be to use another flexible API (like createFrontMatter? #5568) so that they can create this created metadata themselves through some callback.

Up to you, I'm fine merging this PR once ready

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! I'll finish this up then look into the other metadata APIs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

then look into the other metadata APIs.

what do you mean? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then look into the other metadata APIs.

Never mind, I was misinterpreting something. Sorry for confusion.

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 👍

Overall that looks like a very good start, just have some concerns added in comments.


Although I see how it may be useful for some users, I was wondering how users plan to use this exactly?

It looks to me like we implement a feature without a very concrete use case in mind 😅

Would you use this feature on your own doc sites?

But that's fine, the implementation is not too complex and it's optional so I'm ok to add this 👍

lastUpdatedBy={lastUpdatedBy}
/>
)}
<div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't want extra divs if both things are false.

It should never lead to <div></div><div></div> for example

}}>
{'Created{atDate}{byUser}'}
</Translate>
{process.env.NODE_ENV === 'development' && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

wonder if this dev message shouldn't be deduplicated between created/updated cases? We'll potentially display it twice?

Copy link
Contributor Author

@dpang314 dpang314 Jun 30, 2022

Choose a reason for hiding this comment

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

One potential issue with having only one message is that if a user sets created with front matter, but not for last_update, then only last_update will be simulated, while created will use the actual value in development. The dev message will still appear, but there isn't a way to tell that only last_update was simulated. Is this fine?

createdBy,
}: Props): JSX.Element {
return (
<span className={ThemeClassNames.common.lastUpdated}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Created a dedicated themeClassName: user should be able to target each one independently with custom CSS

@@ -381,6 +385,8 @@ declare module '@docusaurus/plugin-content-docs' {
draft?: boolean;
/** Allows overriding the last updated author and/or date. */
last_update?: FileChange;
/** Allows overriding the created author and/or date. */
create?: FileChange;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this name looks a bit weird, but it is consistent with "last_update" 🤪 may be good enough for now?

But maybe we want to use "created" immediately instead, and rename "last_update" to "last_updated" later? (breaking change)

Starting from scratch, is "create" the better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that using "creation" would work well as it is a noun and fits well in existing function names e.g. getFileCreation. This would keep it grammatically consistent with last_update as well.

import {ThemeClassNames} from '@docusaurus/theme-common';
import type {Props} from '@theme/Created';

function CreatedAtDate({
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to share some code across created/updated components?

That feels like duplication, but at the same time, we probably want granularity when swizzling, and avoid creating a weird abstraction? 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since most of the elements have specific ids and/or descriptions, I think that sharing code would probably not be worth it.

@@ -89,6 +91,47 @@ async function readLastUpdateData(
return {};
}

type CreateOptions = Pick<PluginOptions, 'showCreateAuthor' | 'showCreateTime'>;

async function readCreateData(
Copy link
Collaborator

Choose a reason for hiding this comment

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

also feels a bit duplicated, wonder if an abstraction could make sense?

let showedGitRequirementError = false;
let showedFileNotTrackedError = false;

export async function getFileCreate(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it make sense to group getFileLastUpdate + getFileCreate into the same file?

cc @Josh-Cena I thought we had something like gitUtils here in the past

// Wrap in try/catch in case the shell commands fail
// (e.g. project doesn't use Git, etc).
try {
const result = getFileCommitDate(filePath, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this diff command efficient?

it looks like, but didn't run it on a large site / history to see the potential impact

testField({
prefix: 'create',
validFrontMatters: [
{last_update: undefined},
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like a typo: you used last_update instead of create?

});
});

it('docs with create front matter disabled', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

docs with showCreate options disabled?

Copy link
Contributor

@forresst forresst left a comment

Choose a reason for hiding this comment

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

Proposal for the good translation into French from now on

@@ -32,6 +32,9 @@
"theme.common.editThisPage": "Éditer cette page",
"theme.common.headingLinkTitle": "Lien direct vers le titre",
"theme.common.skipToMainContent": "Aller au contenu principal",
"theme.created.atDate": " on {date}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"theme.created.atDate": " on {date}",
"theme.created.atDate": " le {date}",

Proposal for the good translation into French from now on

@@ -32,6 +32,9 @@
"theme.common.editThisPage": "Éditer cette page",
"theme.common.headingLinkTitle": "Lien direct vers le titre",
"theme.common.skipToMainContent": "Aller au contenu principal",
"theme.created.atDate": " on {date}",
"theme.created.byUser": " by {user}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"theme.created.byUser": " by {user}",
"theme.created.byUser": " par {user}",

Proposal for the good translation into French from now on

@@ -32,6 +32,9 @@
"theme.common.editThisPage": "Éditer cette page",
"theme.common.headingLinkTitle": "Lien direct vers le titre",
"theme.common.skipToMainContent": "Aller au contenu principal",
"theme.created.atDate": " on {date}",
"theme.created.byUser": " by {user}",
"theme.created.createdAtBy": "Created{atDate}{byUser}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"theme.created.createdAtBy": "Created{atDate}{byUser}",
"theme.created.createdAtBy": "Créé{atDate}{byUser}",

Proposal for the good translation into French from now on

@slorber
Copy link
Collaborator

slorber commented Aug 9, 2022

@dpang314 do you want to follow-up on this? Otherwise I'll complete your work and move on

@dpang314
Copy link
Contributor Author

@slorber I think that all the issues have been addressed, but I wanted to confirm the best way to handle this issue.

@slorber
Copy link
Collaborator

slorber commented Aug 16, 2022

sorry missed the commits, will review later ;)

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.

5 participants