-
Notifications
You must be signed in to change notification settings - Fork 27
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
Created Running Instructions & Added Contribution Instructions #37
base: master
Are you sure you want to change the base?
Conversation
catrinalim
commented
Sep 25, 2024
- Created a Running section that details how to clone the repo and launch locally
- Added Contribution Instructions including forking, cloning, and raising a PR
@catrinalim This is super awesome! Thank you for taking the time to do this. Let me know when this PR is good to go, and I will give it a review. I think you need to request it the review. Cheers 🍻 |
@Kully Hello! I am attempting to request you as a reviewer but it states I don't have the correct permissions to do so. I have attached an image of the error message: Please let me know if there's anything on my end I can do to get this reviewed. |
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.
@catrinalim A lot of good stuff with this PR. I do hope having these instructions will make it easier for people to contribute to the project.
A few comments and questions.
git clone https://github.com/Kully/pixel-paint.git | ||
``` | ||
Open the pixel-paint folder in the newly cloned folder. | ||
Launch index.html locally. It should open a tab in your broswer. |
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.
Typo: broswer should be browser.
```bash | ||
git clone https://github.com/Kully/pixel-paint.git | ||
``` | ||
Open the pixel-paint folder in the newly cloned folder. |
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.
Please add a slash after pixel-paint: it should look like pixel-paint/.
```bash | ||
git checkout -b <branch-name> | ||
``` | ||
5. Make changes and add your new features/improvements you think benefits the app. Have fun and be creative! |
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.
Can you remove steps 6 - 8 please? These are basic git controls that future contributors should already know about.
Beyond this, I don't think it is always good practice to commit all files at once when working on a feature. It is best to make one small change, then give a specific commit that describes the change, then push.
As an aside, I think it would be great to ask people to use Semantic Commits Messages. I am already doing this on other projects (professional and personal) so would be nice to ask people to do the same here. :)
```bash | ||
git checkout -b <branch-name> | ||
``` | ||
5. Make changes and add your new features/improvements you think benefits the app. Have fun and be creative! |
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.
Can you remove "have fun and be creative!"?
I appreciate the fun suggestion, but I think we should just be focusing on the instructions here.
WDYT?
```bash | ||
git push origin <branch-name> | ||
``` | ||
9. Create your Pull Request: |
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.
Are all these steps necessary for Step 9? Again, this seems like basic git practice that people should know.
It also might be confusing if people's user experiences of Github are different. Mobile vs Web? If Github changes their GUI in the future? etc.