-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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: last updated time in docs #913
Conversation
Deploy preview for docusaurus-preview ready! Built with commit ca7e72f |
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.
Thanks for the PR.
Can you refactor the function to core/utils.js
and optionally write a unit test for it ? Try to make it as general as possible
It can be something like
function getGitLastUpdated(filepath) {
const timeStamp = parseInt(spawn.sync('git', ['log', '-1', '--format=%ct', filepath]).stdout.toString('utf-8')) * 1000;
return new Date(timeStamp).toLocaleString();
}
Edit: In addition, what if Docusaurus user does not use Git ? We should omit the whole last updated
and not display it at all (return null)
Another thing is that the docs are not just in website/versioned_docs
. You should check the docs metadata and find that particular source of docs. (it could be in docs
or website/translate_docs
or website/versioned_docs
.
For example, this current PR couldn't get the last update time of docs
. (See the 'Invalid Date' below)
@endiliey Thank you for your suggestions. Made the suggested changes. :) |
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. Just few nits below before merging and would you mind to add test for getGitLastUpdated
? I think adding test is a very good practice. You can test at least 2 cases, valid file and invalid file
lib/core/DocsLayout.js
Outdated
@@ -110,6 +115,12 @@ class DocsLayout extends React.Component { | |||
</a> | |||
)} | |||
</div> | |||
{updateTime !== null ? ( |
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.
Try Inline If with Logical && Operator which is more declarative
Example
{updateTime && <div>{updateTime}</div>}
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.
@endiliey Thanks for approving. Yes you are right, adding tests would be a great thing to do. But I had a few doubts regarding that.
Firstly, when testing should I compare the timestamp returned to some hard-coded timestamp or just that it's a string. Because the hard-coded timestamp would change if someone changes a doc.
Secondly, if git is not installed then these tests won't work right?
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.
You should use a test doc file (which we wont edit anymore) and put it in __fixtures__
. The test could be a snapshot testing.
Then the second case is to test if there is no git timestamp, the function should return null. Similar to non existing file, return null
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.
@sbansal3096 Excellent for your first PR to Docusaurus 👍
I am wondering if we should have a configuration option for this so folks can decide whether they want the last updated string?
@JoelMarcey Sure! We can have that. By default should we keep it on or off? |
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.
We may have an issue with running npm start
with this PR.
@sbansal3096 is investigating.
Edit by Endilie:
It is caused due to dependencies. Since we add a new npm package, we should add lockfile
@sbansal3096 Just an extra suggestion, try creating new branch when creating a PR, do not send your forked |
@sbansal3096 mind to update the docs for https://docusaurus.io/docs/en/site-config.html#optional-fields for this changes ? And resolve the conflict by merging master / you can rebase |
@endiliey Should I merge all the commits into 1 commit? |
No need @sbansal3096 I'll squash when merging it. LGTM. Are you ready to ship ? |
@endiliey Thanks alot. Yeah I am good to go. :) |
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 approve these changes! 😄
Motivation
Fixes #895
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan