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

polish(v2): improve Docusaurus 1 to 2 migration developer experience #2884

Merged
merged 13 commits into from
Jun 6, 2020

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jun 4, 2020

I recently migrated TS cheatsheets from v1 to v2.

Here are migration doc additions, and improved error messages, that would have helped me save some time.

@slorber slorber requested review from lex111 and yangshun as code owners June 4, 2020 15:42
@slorber slorber marked this pull request as draft June 4, 2020 15:42
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 4, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 4, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 8331763

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

@slorber slorber marked this pull request as ready for review June 5, 2020 15:17
@slorber
Copy link
Collaborator Author

slorber commented Jun 5, 2020

Hi @lex111 @yangshun this is ready to review

Apart docs and better error messages, I also made sure that a navbar link with no defined position actually appears in the navbar (right, as v1), as I was surprised to get my items not appearing anywhere ^^

@yangshun
Copy link
Contributor

yangshun commented Jun 5, 2020

Apart docs and better error messages, I also made sure that a navbar link with no defined position actually appears in the navbar (right, as v1), as I was surprised to get my items not appearing anywhere ^^

This is my bad. It initially had a fallback, but I removed it to make it explicit. I think I was thinking if we could have 'center' as a valid option in future 😂

@slorber
Copy link
Collaborator Author

slorber commented Jun 5, 2020

I'm not agaist making it explicit actually, as long as we have an error saying the position is missing :)

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.

Thank you! Let's use the commit message format for easier PR triaging when releasing. I think this would be under "tag: polish" :D

@yangshun yangshun added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Jun 6, 2020
@yangshun yangshun changed the title Improve DX on v1 -> v2 migration polish(v2): improve Docusaurus 1 to 2 migration developer experience Jun 6, 2020
@yangshun yangshun merged commit 1003a15 into facebook:master Jun 6, 2020
@lex111
Copy link
Contributor

lex111 commented Jun 6, 2020

@yangshun FYI we don’t have a commit type as "polish", instead we need to use a "refactor".

@yangshun
Copy link
Contributor

yangshun commented Jun 6, 2020

But we have a tag for it. Can we add the polish commit type?

@lex111
Copy link
Contributor

lex111 commented Jun 6, 2020

Perhaps it's better (more usual) to replace polish with refactor. https://github.com/facebook/docusaurus/blob/master/lerna.json#L11

@lex111
Copy link
Contributor

lex111 commented Jun 6, 2020

Although I still don't see the big trouble here, the types of commits and lerna labels do not have to match. This is a bit confusing, but we don't have many labels 🤷‍♂️

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: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants