-
Notifications
You must be signed in to change notification settings - Fork 89
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
Contributing guidelines #679
Conversation
CONTRIBUTING.md
Outdated
|
||
1. [Dev workflow](#dev-workflow) | ||
1. [Issues](#1-issues) | ||
2. [Push Requests (PRs)](#2-push-requests-prs) |
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.
Pull* requests
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.
Hehe, my bad. I always mix the two in "PR" :)
CONTRIBUTING.md
Outdated
|
||
Quick overview of the workflow: | ||
|
||
1. bug is found / feature is suggested → new Issue |
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.
check if it exists before creating an issue (worth reminding)
CONTRIBUTING.md
Outdated
Quick overview of the workflow: | ||
|
||
1. bug is found / feature is suggested → new Issue | ||
2. new branch is created with code solving the Issue → Push Request initiated |
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.
pull*
CONTRIBUTING.md
Outdated
- ❎ _"When you click next page in address book nothing happens and 2 pages are hidden please help!!!1!"_ | ||
- **Include as much related information** in Issue's description as you can (the predefined template will help you with that) | ||
- For bugs: what's happening, how to reproduce it, what OS/version are you using, ... | ||
- **Tag the Issue with Labels** to categorize it (is it a `bug` or just an `enhancement` like a new feature?). If you're not sure which label to use, leave them empty, someone else will do that (better no labels at all than wrong ones) |
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.
enhancement is not the same as feature
CONTRIBUTING.md
Outdated
- `feature/..` - for new features, e.g. `feature/coldstaking` | ||
- `gfx/..` - for graphic/UI/UX tweaks, e.g. `gfx/typography` | ||
|
||
When your branch is ready, do your changes and push them to the branch. Be sure to push only the changes related to the connected Issue! Solving multiple Issues is also possible, but it's better to focus on one thing at a time. |
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.
fixing several issues in one PR is forbidden for anyone else than you, GFX designer @AllienWorks
CONTRIBUTING.md
Outdated
|
||
#### 2.2. Commit your code | ||
|
||
- Write meaninful commit messages – don't forget that when your PR is merged, its context is lost. Meaning that once your commits get to `dev` branch, it's harder to say what each commit does just by its name: |
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.
we should mention that the code and commits will be human read with attention and PRs will be refused if they are not described correctly
CONTRIBUTING.md
Outdated
|
||
- **Always set your base branch to `dev`** (never to `master` or others!) – `dev` is our main development branch | ||
- **As always, write short and descriptive title** (you can keep it the same name as your branch) | ||
- **Describe what has been done in description** – what does this PR solves? We usually list all the changes and even include a screenshot/GIF overview of the final result. This greatly helps the Reviewers and Testers as they know what to focus on. |
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.
- picture of the problem
- picture of the solution
CONTRIBUTING.md
Outdated
- **Always set your base branch to `dev`** (never to `master` or others!) – `dev` is our main development branch | ||
- **As always, write short and descriptive title** (you can keep it the same name as your branch) | ||
- **Describe what has been done in description** – what does this PR solves? We usually list all the changes and even include a screenshot/GIF overview of the final result. This greatly helps the Reviewers and Testers as they know what to focus on. | ||
- **Include references to Issues that this PR solves** – Writing `Fixes #[number]` automatically connects this PR to Issue _[number]_ and when this PR gets merged later, the corresponding Issue gets automatically closed. |
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.
writing -> vague. commit message ? PR comment ?
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.
Good point. Summarizing these all in PR's description is the best IMO.
@@ -49,12 +47,12 @@ You can directly interact with the daemon ran by the Electron version. | |||
## Running | |||
|
|||
### Start Electron | |||
* `npm run start:electron:fast` - disables debug messages for faster startup |
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.
replace all npm commands by yarn / remove the deprecated --
can you please also add snyk badge https://github.com/particl/particl-desktop/pull/706/files#diff-04c6e90faac2675aa89e2176d2eec7d8 |
actually don't include snyk i'll merge #706 |
@AllienWorks there's still one change :) |
@pciavald it's not, fixed already :) dunno why, but GitHub thinks the issue is still there, even if the line was removed by other one.. (or am I just blind? – check the changed files) |
yeah perfect @AllienWorks. can I merge this now or was there a reason for it to be for 1.1.1 ? |
Not that I'm aware of. I think we're free to merge; after all this is needed a long time ;) |
Needs proof-reading and fact-checking! Maybe you have better solutions, I'd love to hear them :)