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

Change: improve CONTRIBUTE.md #61

Merged
merged 2 commits into from
Feb 16, 2017

Conversation

KyleScharnhorst
Copy link
Contributor

Noticed a few things that might spice up the contribute doc.

  • Markdown files can have anchor links, so I thought I'd update the "getting started" to point to what it was referencing.
  • Grammar improvements: title capitalization and it's vs its.
  • "All PRs:" was asking to become a header due to the colon and followed list.

Is the section about the changelog still pertinent?

Markdown files can have anchor links, so I thought I'd update the "getting started" to point to what it was referencing. 

Grammar improvements: title capitalization and it's vs its.

"All PRs:" was asking to become a header with the colon and followed list.
Copy link
Member

@TimMikeladze TimMikeladze left a comment

Choose a reason for hiding this comment

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

Thanks. This looks good.

Do you modifying the development section to include this:

Getting started:

npm i

This will link together all the packages.
npm run link

Watch the packages for changes and recompile
npm start

CONTRIBUTING.md Outdated
@@ -4,7 +4,7 @@

The GraphQL Accounts project was intended - since it's inception - to be a community maintained project. We'd love to see you get involved (especially long time contributors from the Meteor community who we've worked with before).
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind rephrasing this so it doesn't include "GraphQL" since we're transport agnostic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy to. I was in the process of adding getting started stuff here

Copy link
Member

Choose a reason for hiding this comment

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

Also, the devel branch isn't being used to keep the latest development code so references to that should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it reference master? Or what are you thinking?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's implied. Line 11 has you explicitly checkout devel

@TimMikeladze
Copy link
Member

We haven't been updating the changelog. If we're gonna keep that in the guidelines then moving forward let's make an effort to do it.

@TimMikeladze
Copy link
Member

The rest of the repos under js-accounts could use similar, if not identical CONTRIBUTING.md files.

@KyleScharnhorst
Copy link
Contributor Author

This project needs a CHANGELOG.md then? and probably others?

devel doesn't always have the most pertinent development code. Replaced with general references so that the contributor can decide which branch they are interested in. 

Didn't feel clean to invade the getting started list. It was just so tidy. There is now a development header that lists useful information for starting project development. This may or may not be what we want.
@TimMikeladze
Copy link
Member

This looks good. Ready to merge?

@KyleScharnhorst
Copy link
Contributor Author

Yessir

@TimMikeladze
Copy link
Member

Thank you!

@TimMikeladze TimMikeladze merged commit bb892ea into accounts-js:master Feb 16, 2017
@KyleScharnhorst
Copy link
Contributor Author

Glad I could help. Thanks for the learning opportunity! I was originally going to see about increasing code coverage, but got caught up along the way, ha.

Aetherall pushed a commit that referenced this pull request Mar 13, 2018
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.

2 participants