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

Graded! #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Graded! #2

wants to merge 1 commit into from

Conversation

jagreene
Copy link
Collaborator

This is an awesome and fun app. I found myself losing at least two hours reading articles while I was reading this. Bravo! I think your app would have benefited a lot from some of the more advanced react tools, like a flux implementation and react-router, but I recognize that that would have been difficult to learn in the time allotted. My main wish is that the code had been more well commented, you did a lot of cool things, but deciphering them took a substantial amount of time. Besides that I left a few nitpicks and gripes inline.

Tiny code cleanliness suggestion: you've got a bit of whitespace inconsistency in that some of your lines end with whitespace and some don't. I think it's good practice to make sure there's no extra whitespace at the end of each line of code -- Sublime can help you with this (http://nategood.com/sublime-text-strip-whitespace-save). The reasoning behind my opinion: if you're working in a development workflow where you code review other people's diffs and you don't all agree about whitespace, the diffs will probably include some just-whitespace changes. Not the end of the world, but I think it's easier to review when I know all of the changes in the diff are significant (i.e. actual code changes) -- that way I don't need to filter for "that's just a whitespace change" vs "oh, that change is actually part of the new feature or the bug fix". Similar thing applies to newlines at the end of files -- it's convention to make sure there is one because if there's not a newline at the end of a file and you add more lines at the end, the previously-last line will look like it changed because it now contains a newline. Again, Sublime can help you with this (https://forum.sublimetext.com/t/make-saving-newline-at-eof-the-installation-default/9842) Another common thing is to convert tabs to spaces which render more consistently across development environments, this can be found in sublime settings too. This is such a nitpicky detail, and I'm sure you'll find different development workflows which have different rules about newlines/trailing spaces -- mentioning my preferences just so you're aware that people have opinions about these things. :)

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.

1 participant