search

adrianno33 / time-organizer

1 stars 1 forks source link

some comments & suggestions #4

Open LucasPiedrahita opened 2 years ago

LucasPiedrahita commented 2 years ago

Saw your post on reddit (https://www.reddit.com/r/reactjs/comments/tw8lzn/could_someone_do_a_code_review_on_my_first_react/) & had some suggestions. I can keep adding suggestions to other files if that would be helpful for you. Feel free to ask about anything in this PR. I wouldn't recommend actually merging it, I just wanted you to be able to see the comments in context.

adrianno33 commented 2 years ago

Thank you for your help! Great idea with changeCompletion function, it's much more readable now and I didn't notice the redundancy, so I'll probably remove isComplete completely. Your remarks were very helpful, so if it wouldn't bother you too much, I'd love it if you would review other files too.

LucasPiedrahita commented 2 years ago

Thank you for your help! Great idea with changeCompletion function, it's much more readable now and I didn't notice the redundancy, so I'll probably remove isComplete completely. Your remarks were very helpful, so if it wouldn't bother you too much, I'd love it if you would review other files too.

Sure, I'll add to this when I get some time! Also remember that everyone reviewing your code is just another developer who started the same place you did and doesn't know everything, so if you don't understand or disagree with a suggestion, it could be that the person reviewing is wrong (me included) or that there are multiple correct ways to do something. Question everything until it makes sense to you 🙂

LucasPiedrahita commented 2 years ago

@adrianno33 I Went through all the js files with some suggestions, let me know if you have any questions.

I'm not going to go through your CSS files, but overall the styling looks good, there are just a few areas for improvement that could be quick wins for your UI:

  1. Could you use CSS grid to layout your timers so that you can have multiple per row on larger screens? Also the timers seem a bit big to me, but that could be a personal preference.
  2. Your hover states are not accessible because the white text on light pink background doesn't provide enough contrast.
  3. Could you dynamically generate the manage table's max-height css property so that if can take advantage of the whole screen height before scrolling? When viewing on a large screen it seems odd to only be using half the screen for the table but then have to scroll within the table (I added a bunch of timers lol).