@BParadowski
Posted
Okay, after taking a look at your code I have some suggestions.
-
Naming: your names are not descriptive. To be frank I have no idea what most of your code does. Component name like "ActionButton.tsx" doesn't say anything about its purpose (every button has some action/function after all). The same goes for props the like of "isReplyReply" or "onIsReplyingChange".
-
Continuing in the spirit of code readability: destructuring assignment. It allows us to do stuff like changing this:
const onCommentVoteChange = props.onCommentVoteChange;
const commentIdToChangeVote = props.commentIdToChangeVote;
const onReplyVoteChange = props.onReplyVoteChange;
const replyIdToChangeVote = props.replyIdToChangeVote;
into this:
const {
score,
onCommentVoteChange,
commentIdToChangeVote,
onReplyVoteChange,
replyIdToChangeVote,
} = props;
-
Components: you have 4 different button components which fundamentally are only visually different. A more sensible approach would be to create one component with variants.
-
Lastly, prop drilling and logic placement. There is no reason to define comment adding logic on the button! There is also no reason to make delete modal a child of the delete button... Aim to place the logic where it makes sense.
If I were you I would focus mostly on code-readability and fighting your urge to prop-drill. I must confess that I still have no idea what is going on in most of your code.
Marked as helpful
@taufiqmahdi
Posted
@BParadowski
First, I want to say thank you so much for giving me the feedback on this challenge.
- Naming: Thank you for the feedback, I thought myself that that has been the best way to describe what something does, turns out is not, that's what experience teaches you I think. I will work on it in the future.
- Continuing in the spirit of code readability: I know! I was thinking there must be an easier way to do this, turns out there is. I will apply this in my future projects!
- Components: Thats a great idea! But i wasnt sure how to implement it. Also, they had different functionality so thats why i was thinking to differentiate it. But maybe, give it a props that say what is the variant and invoke the function based on the variant, right?
- Prop drilling and logic placement: Yeah... But i dont know how to place the function that is when the button is clicked, the function in two upper component invoked... Thats why I do that.. Yeah thats correct too... Maybe the solution is to place the correct calling function in the correct component, right?
Overall I see your point, thank you so much for the feedback, im glad you spend your time giving me a feedback :D
@BParadowski
Posted
@taufiqmahdi
With regards to components: Another thing worth mentioning is that you do not need to make every a component (nor should you). When thinking about dividing your app into components ask yourself questions like:
- Does this piece of code encapsulate some logic/function?
- Am I going to reuse it in different places?
- Does moving this piece of code into another file greatly improve code-readability?
or, once you understand RSC and Next 13 app directory:
- Is this an island of client-side code on otherwise server-side generated html?
If the answer to one of these is yes, then consider making it a component. For example, when looking at this project I would think mainly about 2 components:
- <Comment> component which optionally recursively renders replies to it and a new comment form.
- <NewCommentForm> self explanatory
This would also answer you questions about logic placement. Where should new comment placing logic go? into the form. etc.
I also think that many problems in this challenge stem from you trying to simulate the backed. In reality this thing would be a bit complicated (involving authentication and database syncing with something like ReactQuery). I suggest you take a step back, understand design patterns of React and UI first and only then move to learning those, for real this time.
Happy coding!
Marked as helpful
@taufiqmahdi
Posted
@BParadowski
Thank you again for the feedback.
True that, I find myself stuck that I think this component is used few times but in different depth of components, that is where I am confused.
I see, maybe I was making everything a component as small and as detailed as possible, which should have been not.
Thank you for the suggestion, where can I learn that? I was thinking at this stage of my understanding in Web Development, the more I code then the more I learn, just a matter of time. Was it wrong or should I work on my basic fundamental first? Thank you
