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

Update contributors guide and issue templates #5962

Merged
merged 2 commits into from Jan 16, 2019

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jan 6, 2019

This adds the following:

  • Suggest to ask maintainers about changes ahead of time
  • Suggest that new methods be made private
  • Suggest to add unit tests

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:

  • Remove the note to "Only change the individual files in /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 this
  • Combined the notes about running the linter and tests into a single line since gulp test covers both of these cases. There's a table down below that explains gulp lint, gulp unittest, etc. if users want to run only a subset of the checks
  • Remove note about tabs vs spaces. This is covered by the line saying to run the linter

@simonbrunel
Copy link
Member

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)?

@benmccann benmccann force-pushed the contributing branch 2 times, most recently from a20536c to c49970f Compare January 7, 2019 19:11
@benmccann benmccann changed the title Suggest new methods to be added as private Update contributors guide Jan 7, 2019
@benmccann benmccann force-pushed the contributing branch 2 times, most recently from 14f281d to 6a1a965 Compare January 7, 2019 19:17
@benmccann
Copy link
Contributor Author

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.

@simonbrunel
Copy link
Member

I think we just need to create the associated files in the .github folder.

@benmccann benmccann force-pushed the contributing branch 2 times, most recently from 4b2edad to 99226ce Compare January 7, 2019 21:36
@benmccann
Copy link
Contributor Author

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.

@benmccann benmccann changed the title Update contributors guide Update contributors guide and issue templates Jan 7, 2019
@simonbrunel
Copy link
Member

@benmccann I think we should be able to see it in action if you enable issues on your repository.

@benmccann benmccann force-pushed the contributing branch 3 times, most recently from fffd219 to 4665b7c Compare January 8, 2019 15:56
@benmccann
Copy link
Contributor Author

I pushed it to my master branch @simonbrunel. Check it out here: https://github.com/benmccann/Chart.js/issues/new/choose

@simonbrunel
Copy link
Member

simonbrunel commented Jan 9, 2019

https://github.com/benmccann/Chart.js/issues/new?template=FEATURE.md

It doesn't work for me, I see the fallback template (ISSUE_TEMPLATE.md)

etimberg
etimberg previously approved these changes Jan 11, 2019
Copy link
Member

@simonbrunel simonbrunel left a 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: *)

.github/ISSUE_TEMPLATE/BUG.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/BUG.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/SUPPORT.md Outdated Show resolved Hide resolved
@simonbrunel
Copy link
Member

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 :)

etimberg
etimberg previously approved these changes Jan 15, 2019
Copy link
Member

@etimberg etimberg left a 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

@benmccann
Copy link
Contributor Author

Ok. I've updated the formatting

@simonbrunel simonbrunel merged commit 740e087 into chartjs:master Jan 16, 2019
@simonbrunel
Copy link
Member

Thanks @benmccann

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants