fac-14 / Brickworks-Tollington_Project

MIT License
2 stars 5 forks source link

Documentation (Code Review) #79

Open jamiecoe opened 5 years ago

jamiecoe commented 5 years ago

ReadMe

Great start! Nice to see you've already got a license / link to site / info on deployment & technical commands 👍

Some more things you could add:

This inFact one is nice example of a ReadMe that tells the story of project in lots of detail.

ReadMes are great for your portfolios - the repository will not only show off your code but the entire project process (design / user-testing / project managing / prototyping) to future employers or free-lance clients.

Issues

Great to see you labelling issues! It would be useful to label PRs as well (eg: ‘in progress’ if it’s still being worked on and ‘awaiting review’ when you want the team to review it). That way, the team know when a PR is ready.

Excellent use of ‘Projects’ board 🎉 Really nice to be able to see progress of Issues.

Milestones are handy for grouping issues together into Sprints. It makes it easy to track what you are working on in a current Sprint and the progress bar shows how far you’re off from finishing.

I see you’re referencing other issues here. You can link to other issues or PRs using a hashtag with the number of the issues/PR (eg: #53).

Commit messages

For commit messages, focus on why you've made the commit, not what is in the commit (the code itself should show this).

A good example of one of your commit messages that focuses on the why:

clean up database files as we are using airtables now 

This is great because it explains why you made the changes - which is easy for me (as an outside developer) or you in a few months/years time to understand why you made that change. Remember the why is the important information that can easily get lost, so documenting it with commit messages is really helpful.

Also, try to avoid the temptation to have multiple things in one commit. Eg:

implement displaying one-detailed-event on clicking div and implement airtables. 

What if you wanted to roll back on the one-detailed-event stuff, but keep airtable? If these were two separate commits, you could easily role back one and keep the other.

MissArray commented 5 years ago

Hi Jamie,

There's already a README. with the commands for running tests at the bottom, but of course, as you point out, the file is still incomplete. Working on it...