GermanS04 / Kudos-board

0 stars 0 forks source link

Week 3 Code Review - Mountain #2

Open mountainhenning opened 1 week ago

mountainhenning commented 1 week ago

Overall looks great, I just have some minor "gotchas" that I know the linter we use at Meta will yell at you about LOL.

Also, doing this through the issues format is a pain so I'll just add comments to Boards.jsx, but these apply to each file overall, again great job, this is not easy stuff 😄


General note #1: I'm not sure if they taught you different ways of typing for Javascript, but we use something called Flow, you may want to learn about it early on.

General note #2: add comments to literally everything, it is a good habit to build and will definitely impress everyone that reads your pull requests.


https://github.com/GermanS04/Kudos-board/blob/6756ba6b014a142d3dc087a16036e628e7e62102/Frontend/kudos-board/src/components/Boards.jsx#L9

Move this to the import statements.


https://github.com/GermanS04/Kudos-board/blob/6756ba6b014a142d3dc087a16036e628e7e62102/Frontend/kudos-board/src/components/Boards.jsx#L12-L13

Don't make these into states, just use the props themselves, treat state and props like oil and water lol.


https://github.com/GermanS04/Kudos-board/blob/6756ba6b014a142d3dc087a16036e628e7e62102/Frontend/kudos-board/src/components/Boards.jsx#L8

Either rename the boardData prop to board or just use boardData.


https://github.com/GermanS04/Kudos-board/blob/6756ba6b014a142d3dc087a16036e628e7e62102/Frontend/kudos-board/src/components/Boards.jsx#L15

onDelete should be something like "sendDeletion".


https://github.com/GermanS04/Kudos-board/blob/6756ba6b014a142d3dc087a16036e628e7e62102/Frontend/kudos-board/src/components/Boards.jsx#L18

You will want your error to actually "throw", if it just logs the error, then its very easily missable, check out this guide.


https://github.com/GermanS04/Kudos-board/blob/6756ba6b014a142d3dc087a16036e628e7e62102/Frontend/kudos-board/src/components/Boards.jsx#L34

Make a separate const for '/default_image.jpg'.


https://github.com/GermanS04/Kudos-board/blob/6756ba6b014a142d3dc087a16036e628e7e62102/Frontend/kudos-board/src/components/Boards.jsx#L22

Empty strings are tricky, utilize an external function or check the length of the string (reference)


https://github.com/GermanS04/Kudos-board/blob/6756ba6b014a142d3dc087a16036e628e7e62102/Frontend/kudos-board/src/components/Boards.jsx#L34

Use Image from react, general note, I recommend getting use to using components defined from external packages (React should have most of the ones you would need for this project) rather than using HTML tags, it will save you a lot of headache in more complicated projects.


GermanS04 commented 1 week ago

Hey thanks for the feedback, I just have a few doubts/inquiries.


About Flow, I was talking to Kevin about it and he mentioned that maybe Flow is out of the scope for the Meta U internship due to the short period of time to make the capstone project, I still read a bit of the documentation on what it does and it seems like a really cool tool, I'll definitely look more into it after the internship.


For the props and useState, thank you for pointing that out I had no idea they shouldn't be combined lol.


Regarding the catch error I looked into the guide you linked, I found it really useful and I'll do it for my capstone project, I started working on it and I think I kinda did something similar (if you have the time I would like to show you to know if that is what you're referring to)


Also, after the comment of the string comparison, I've been using the .length for the string instead of the empty string, and combining it with the .trim so I can also check if it's only whitespaces, thank you for the recommendation helped a lot.


On the Image from react, I had a doubt cause the link is for image component from react native, does it still work on react even if it's from react native? For the capstone I'm working with the Next JS framework for React and it has a Image component, I think that's also an alternative.


Thank you again for the feedback, I'll be sure to get the most out of it for the capstone project.