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

chore(welcome bot): add welcome bot config #5313

Merged
merged 3 commits into from
Jul 11, 2018
Merged

chore(welcome bot): add welcome bot config #5313

merged 3 commits into from
Jul 11, 2018

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Jul 11, 2018

No description provided.

💖 Thanks for opening this pull request! 💖
Things that will help get your PR across the finish line:

- Run `npm run lint -- --errors` locally to catch formatting errors earlier.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this should just mention that npm run test passes locally?

Copy link
Contributor

Choose a reason for hiding this comment

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

it could also be worth mentioning that they should keep an eye on travis to make sure tests pass

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, I guess the question is how much we want to mention in this comment. We do already mention tests as well in the next bullet point, so, maybe that's enough reminder.


- Run `npm run lint -- --errors` locally to catch formatting errors earlier.
- Include tests when adding/changing behavior.
- Include screenshots and animated GIFs whenever possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do they need to do this? Won't netlify do this for us most of the time? Should we mention that here somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes sense, particularly for UI changes. Netlify gives us a live example but we still can't be sure what the intent and broken behavior was without it.
Also, screenshots and gifs can help debug stuff that aren't directly UI changes but rendering issues and what not.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, yeah your right.


# Comment to be posted to on first time issues
newIssueWelcomeComment: |
👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
Copy link
Contributor

Choose a reason for hiding this comment

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

You may also want remove the part about the steps to reproduce here and just mention that they should follow the issue template (which includes the steps to reproduce).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be good to keep because it's less likely to get ignored as it could be if it's in a link.

# Comment to be posted to on first time issues
newIssueWelcomeComment: |
👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
To help make it easier for us to investigate your issue, please follow the [contributing guidelines](https://github.com/videojs/video.js/blob/master/CONTRIBUTING.md) and [issue template](https://github.com/videojs/video.js/blob/master/.github/ISSUE_TEMPLATE.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just link here instead of the issue template at all. https://github.com/videojs/video.js/blob/master/CONTRIBUTING.md#filing-issues s

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, maybe just contributing.md is enough. electron only links there.

Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

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

Looks awesome. One minor comment.


# Comment to be posted to on first time issues
newIssueWelcomeComment: |
👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move the location of "If you're reporting a 🐞 bug, please make sure you include steps to reproduce it." a bit.

Thanks for opening your first issue here! We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
To help make it easier for us to investigate your issue, please follow the [contributing guidelines](https://github.com/videojs/video.js/blob/master/CONTRIBUTING.md) and [issue template](https://github.com/videojs/video.js/blob/master/.github/ISSUE_TEMPLATE.md). And if you're reporting a 🐞 bug, please make sure you include steps to reproduce it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only issue I see with this is that if it's in the end, people may drop off before they get there :)


# Comment to be posted to on first time issues
newIssueWelcomeComment: |
👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be similar to the pr text 👋 Thanks for opening your first issue here! 👋 and then the rest on the next lines


Things that will help get your PR across the finish line:

- Run `npm run lint -- --errors` locally to catch formatting errors earlier.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually since we have a git hook for this, it shouldn't be possible to push with linting errors

Copy link
Member Author

Choose a reason for hiding this comment

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

you can turn it off with --no-verify

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah but then its pretty much your own fault haha

Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

If we want to change the npm run lint you can. I think this is good other than that, and I don't think it matters if we keep it or remove it.

@gkatsev gkatsev merged commit e637768 into master Jul 11, 2018
@gkatsev gkatsev deleted the welcome-bot branch July 11, 2018 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants