-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Update contributors guide and issue templates #5962
Conversation
I would certainly also add a note about not opening PR for major architecture changes, refactor, introduction of major features or API changes, without first getting approval on the expected API and/or implementation, either via creating a new ticket or via our slack #dev channel (not sure how to formulate). I noticed that GitHub enhanced their issue creation workflow and I really like it (e.g. rollup). Maybe we could setup something similar (bug / enhancement / support)? |
a20536c
to
c49970f
Compare
14f281d
to
6a1a965
Compare
Ok @simonbrunel. I added a note to ask about major changes before making them I also like the idea of the issue types. I took a look at trying to set that up as well, but I believe that additional GitHub permissions are required, so you or @etimberg will need to do or grant someone else permission to manage that. |
I think we just need to create the associated files in the |
4b2edad
to
99226ce
Compare
Yeah, I think you're right. I just can't use the interactive editor on GitHub, but was able to create them manually. Take a look and let me know what you think. |
99226ce
to
dd0a1c4
Compare
@benmccann I think we should be able to see it in action if you enable issues on your repository. |
fffd219
to
4665b7c
Compare
I pushed it to my master branch @simonbrunel. Check it out here: https://github.com/benmccann/Chart.js/issues/new/choose |
https://github.com/benmccann/Chart.js/issues/new?template=FEATURE.md
|
4665b7c
to
3b74c5f
Compare
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.
Looks really good @benmccann (I like the labels: 'type: *
)
3cf0bbb
to
5669646
Compare
5669646
to
a50402d
Compare
Looks good, just a minor formatting change and I think we are good to go. @benmccann Can you push changes as regular commits (no squash + force-push), it's easier to review 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.
Looks good. I'm happy to merge once the minor formatting issue @simonbrunel identified is fixed
Ok. I've updated the formatting |
Thanks @benmccann |
This adds the following:
Since a lot was added, I also wanted to try to keep it concise enough that everyone will read it, and so removed some information that was unnecessary or covered in another way:
/src
". I'm not quite sure what this means. I believe it means don't check in the built files. We used to have these checked in prior to v2.2. E.g. https://github.com/chartjs/Chart.js/tree/v2.1.6/dist. We also want people to change tests, docs, and samples when appropriate, so I think it's clearer to remove thisgulp test
covers both of these cases. There's a table down below that explainsgulp lint
,gulp unittest
, etc. if users want to run only a subset of the checks