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

Show warning console if version.js file is missing #678

Merged
merged 2 commits into from
May 21, 2018

Conversation

sujono91
Copy link
Contributor

Motivation

As discussed in #369, we have to give better error message if version.js file is missing in pages/en directory.

The solution was to add warning console if someone ran the version script before creating the versions.js page

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Warning console example if version.js file is not found
screen shot 2018-05-20 at 10 33 46 am

Success example if version.js file is found
image

Run yarn test and it pass all unit test

Related PRs

No

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 20, 2018
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented May 20, 2018

Deploy preview for docusaurus-preview ready!

Built with commit 86615cd

https://deploy-preview-678--docusaurus-preview.netlify.com

@endiliey
Copy link
Contributor

Some suggestion. Instead of just a warning when version.js is missing, will it be better to prompt user if they want to create a default version.js ? This is more from user experience (UX) perspective. Showing https://docusaurus.io/docs/en/versioning.html as a reference guide might be useful as well. WDYT?

@sujono91
Copy link
Contributor Author

sujono91 commented May 20, 2018

@endiliey i agree with u, but can u give me the example of how to create a default version.js?
We can include https://docusaurus.io/docs/en/versioning.html in CLI as well for their guide

@JoelMarcey @yangshun do u've any comments about this?

@endiliey
Copy link
Contributor

Refer to https://docusaurus.io/docs/en/versioning.html, running yarn examples versions create an example pages/en/versions.js

yarn examples versions is actually just running node ../lib/copy-examples.js versions https://github.com/facebook/Docusaurus/blob/aae106c018667a3787726f7744ce14ccb2b68ef1/lib/copy-examples.js#L97-L122

@supasate
Copy link

supasate commented May 20, 2018

Showing https://docusaurus.io/docs/en/versioning.html as a reference guide is useful and nice to be included in this PR.

For prompting and creating a default version.js, should we separate into a new issue/PR? So, this PR focuses only on a better error message and the new issue/PR focuses only on the new functionality to create a default versions.js if not exists.

@endiliey
Copy link
Contributor

Yeah I agree. Showing https://docusaurus.io/docs/en/versioning.html as a reference guide is more important. Nice work 😄

@sujono91
Copy link
Contributor Author

@endiliey @supasate cool.. let me add the link then 😄

@sujono91
Copy link
Contributor Author

sujono91 commented May 20, 2018

@endiliey @supasate @JoelMarcey @yangshun
My Suggestion:

No version.js file founded!
You should create your version.js file in pages/en directory.
Please refer to https://docusaurus.io/docs/en/versioning.html

if everything's ok, I'll continue to add the link message

@JoelMarcey
Copy link
Contributor

@sujono91 That definitely seems reasonable to me. Much better than what we have now -- which is nothing 😄. We can always do any minor tweaking later, if we want.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Just some minor comments before we can merge this. Thanks a lot @sujono91!

lib/version.js Outdated
if (!hasVersionFile) {
console.error(
`${chalk.yellow(
'No version.js file founded!'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be versions.js or version.js? The code checks for versions.js but the error message suggests version.js.

Grammatical nit - The error message should be "No versions.js file found" not founded.

Could you also shift the const hasVersionFile declaration to just above the check here? We would like to co-locate the variables to where they are being referenced, especially when this file is only being referenced once.

lib/version.js Outdated
console.error(
`${chalk.yellow(
'No version.js file founded!'
)}\nPlease create your version.js file in pages/en directory.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Sync filename with above.

@sujono91
Copy link
Contributor Author

Updated @yangshun @JoelMarcey

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Looking good!

@yangshun yangshun merged commit 436a3d0 into facebook:master May 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants