@ChurrinChurron
Submitted
Any suggestions for improving the React code would be appreciated. Thanks!
@BParadowski
@ChurrinChurron
Submitted
Any suggestions for improving the React code would be appreciated. Thanks!
@BParadowski
Posted
Hey,
after taking a look at your code it looks like you are writing Vanilla JS in React. Using DOM API like querySelector() in React is generally a last resort. Since you are bringing in a UI framework you should actually use it.
The main problem with your implementation, from a conceptual standpoint, is that you have 2 sources of truth governing what gets rendered - React and Css classes. Let's take a look at your e-mail validation error message:
<div className='form__text'>
<label htmlFor="email">Email address</label>
<span>Valid email required</span>
</div>
You trigger the visibility of this div by changing the class. In React, this same functionality should be achieved by declaring an 'isErrorVisible" boolean variable using useState() and changing the code as follows:
{isErrorVisible && (
<div className="form__text">
<label htmlFor="email">Email address</label>
<span>Valid email required</span>
</div>
)}
And of course changing the state in your submit function. You have only one class on your element this way and no reason to query any selectors. (You can also conditionally trigger classes based on the state if you really need it)
Hope this helped
Marked as helpful
@sariodesign
Submitted
I would like advice about:
@BParadowski
Posted
Hey,
In my version of this challenge I defined the backgrounds in tailwind.config.js (because I wanted to have control of the background's position with css) and later applied them based on the page url, not props. You can take a look at the implementation here. I find it elegant enough.
My advise with regards to components would be to have fewer of them and to create them only when they are actually needed. The same goes for props. Let's take a look at your social media links component in the footer.
First of all, making it a component makes sense (it serves some dedicated function and can be separated from the rest of UI) but it is not necessary since you never reuse it. With regards to props however - it needs none. There is no reason to generate a (potentially different) list of social media links since there aren't that many of them. You should have simply hard-coded the links and images - no problems with props that way.
The last piece of feedback I'd like to give you is to treat Tailwind more like a design system. You are greatly overusing arbitrary values in your code (the things inside []
). By doing that you are defeating much of the purpose of using Tailwind as a framework. Recommendation: Less pixel-perfection and more of using the tools provided.
Happy coding
Marked as helpful
@taufiqmahdi
Submitted
@BParadowski
Posted
Hello again,
import image from "/assets/image...."
It gets transformed into StaticImageData type and you can place this object into "src" attribute of <Image> (height and width properties are then inferred automatically)Marked as helpful
@taufiqmahdi
Submitted
@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
@jetskeeter1
Submitted
This is probably the most fun I have done, or rather it was relatively getting easier for me. Although, I did not finished it fast enough, but I did my best to make it the same as the image.
Feed back is appreciated!
@BParadowski
Posted
Hey, off the top of my head I have 2 suggestions:
.container {
width: min(
var(--container-max-width),
100% - var(--container-margin-inline-min) * 2
);
margin-inline: auto;
}
(the --container-margin-inline-min custom property changes based on screen size of course).
Marked as helpful