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(v2): add notifications for commands #3074

Closed

Conversation

taylorallen0913
Copy link
Contributor

Motivation

I found this really cool library for dealing with notifications, so I thought why not implement it here. I've implemented notifications for start, swizzle, build, and deploy so far.

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

Start:

start

Swizzle:

swizzle

Build:

build

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 18, 2020
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit 2fba092

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

@anshulrgoyal
Copy link
Contributor

anshulrgoyal commented Jul 19, 2020

I find notification a bit annoying. Maybe adding config to disable it might be good.

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

Some persons might like such notifications but it's not the case for all users.

I'd rather have this feature being an opt-in feature, that you can enable with a configuration setting (that would be documented, and false by default).

@slorber slorber marked this pull request as draft July 29, 2020 17:38
@slorber
Copy link
Collaborator

slorber commented Jul 31, 2020

@taylorallen0913 tell me if you are interested to update the PR, and making this feature optional.

@taylorallen0913
Copy link
Contributor Author

I would be willing to continue this feature, but I am not sure what config you are intending to hold this in. Where would you suggest putting the setting?

@anshulrgoyal
Copy link
Contributor

I would be willing to continue this feature, but I am not sure what config you are intending to hold this in. Where would you suggest putting the setting?

I think adding a flag to each command is a good place.
eg: yarn build --notify

@slorber
Copy link
Collaborator

slorber commented Aug 3, 2020

I'd rather have a notification: true/false (false by default) in site config than a new cli option. People can use env variables if they want it to be dynamic anyway.

@slorber
Copy link
Collaborator

slorber commented Aug 20, 2020

Hi @taylorallen0913 , I'm closing for now but if you want to do the changes I'll re-open

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.

5 participants