Add Checklist to PRs #1041
No Reviewers
Dismiss Review
No due date set.
Dependencies
No dependencies set.
Reference: privacyguides/privacytools.io#1041
Reference in New Issue
Block a user
Delete Branch "patch-2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description: Many users don't appear to be reading or know how to find the contributing guidelines.
My PR just adds some check boxes that may be helpful for new users.
It also adds a link to CONTRIBUTING.md :)
Deploy preview for privacytools-io ready!
Built with commit
8b9d714bfehttps://deploy-preview-1041--privacytools-io.netlify.com
@@ -7,3 +7,2 @@<!--## Screenshots#### Check List <!-- Please add an x in each box below, like so: [x] -->I think this is reduntant as when you open a PR, GitHub suggests you on the right to read the contributing guidelines. It also appears somewhat formal to me.
I am not sure if this is that good idea as it could easily cause a merge conflict if multiple users were submitting PRs to add multiple apps/services simultaneously. I think it should possibly be something for a team member to update (but it needs a PR with approval)? Ideas @BurungHantu1605 ?
I think this should be "Project is preferably..."
I wonder if this could be removed as Netlify handles that?
@@ -7,3 +7,2 @@<!--## Screenshots#### Check List <!-- Please add an x in each box below, like so: [x] -->AFAIK it will only cause a merge conflict if people edit the same line, which seems unlikely since they'll likely be editing different sections of the file. But in any case merge conflicts are relatively easy to solve when a team member merges them. I would however make the following grammatical changes:
I think this is fine to include if we're doing this, because it means people need to affirm that they have read it directly.
I would add instructions for those unfamiliar with Markdown.
This is redundant because of line 17.
LGTM once open discussions are resolved 👍
@@ -7,3 +7,2 @@<!--## Screenshots#### Check List <!-- Please add an x in each box below, like so: [x] -->@@ -7,3 +7,2 @@<!--## Screenshots#### Check List <!-- Please add an x in each box below, like so: [x] -->fixed in latest commit.
Thx
@@ -7,3 +7,2 @@<!--## Screenshots#### Check List <!-- Please add an x in each box below, like so: [x] -->Software must actually be free software:
https://github.com/privacytoolsIO/privacytools.io/blob/master/.github/CONTRIBUTING.md#software
But, ya that description makes more sense.
Not really, it makes it easier to evaluate projects.
If they have a bazillion bugs, for example, then we may want to look at that.
Plus, for much of the software (like browsers) it has a Github button.
This makes it easier to add this if it wasn't done already.
Yeah, good point @ggg27, that could save a few clicks for reviewers to quickly check out the project’s source code.
To keep this MR moving along I'll resolve this discussion. I think this line is fine to keep in the checklist for now unless you think otherwise @JonahAragon.
This is looking good, just some grammatical changes I would make mainly before approving.
@@ -4,3 +4,3 @@Resolves: #none <!-- The number of the issue that is resolved by this pull request. If there is none, feel free to delete this line -->Resolves: #none <!-- The number of the issue that is resolved by this pull request. If there is none, feel free to delete this lineThis entire section (L7-L10) should just be removed entirely.
@@ -4,3 +4,3 @@Resolves: #none <!-- The number of the issue that is resolved by this pull request. If there is none, feel free to delete this line -->Resolves: #none <!-- The number of the issue that is resolved by this pull request. If there is none, feel free to delete this lineThanks
@JonahAragon @nitrohorse I believe everything should be fixed
LGTM
LGTM
Since we have this open PR already, could we add the below to mitigate confusion from new contributors that open PRs before a discussion has taken place? @Mikaela @JonahAragon
I've added the checkbox for ensuring there is an associated discussion/Issue. With three +1's, once the build passes, I think we can go ahead and finally merge this.