@jebbaited
@dalnoki
Submitted
That was my first full React project - since I only started with this technology, I'd like to know more if there are any code smells in the code right now and I want to know if there are more elegant solutions for certain parts.
@jebbaited
Posted
Hi!
1: Do not use create-react-app. Use Vite instead.
2: Follow DRY (Don't repeat yourself) principle. You use the same array as initial state twice. Create it outside of component and then pass to useState().
const [allTodos, setAllTodos] = useState([
{
description: "Jog in the park",
isCompleted: false,
id: 1,
},
{
description: "Walk the dog",
isCompleted: false,
id: 2,
},
{
description: "Walk the dog",
isCompleted: false,
id: 3,
},
]);
Same here
setAllTodos([
...allTodos,
{
description: `${value}`,
isCompleted: false,
id: Date.now(),
},
])
setFilteredTodos([
...allTodos,
{
description: `${value}`,
isCompleted: false,
id: Date.now(),
},
])
3: Why do you pass params to function and don't use them? Use can replcae 'e' with an underscore '_' as a placeholder for function parameter that is not used.
const dragStart = (e, index) => {
dragItem.current = index;
};
const dragStart = (_, index) => {
dragOverItem.current = index;
};
But in this case it is better to not pass 'e' at all. Remove this param. In several component I found such functions. Example:
onDragStart={(e) => dragStart(e, index)}
onDragEnter={(e) => dragEnter(e, index)}
4: For many conditions in classnames I'd use library CLSX or classnames. Because some statements are unreadable. Like this one:
className={`${
currentTodo.isCompleted === true ? "todo-item--completed" : null
} todo-item ${
currentTheme === "light" ? "todo-item--light" : "todo-item--dark"
} ${
currentTodo.isCompleted && currentTheme === "light"
? "todo-item--completed--light"
: ""
}
${
currentTodo.isCompleted && currentTheme === "dark"
? "todo-item--completed--dark"
: ""
} round`}
5: Unnecessary ref.
const searchInput = useRef(null)
6: If you need a unique id just use React hook useId() DOCS instead of Date():
{
description: `${value}`,
isCompleted: false,
id: Date.now(),
},
7: For theme toggling use useContext() hook. There are a lot of example you can find. One of them is Medium. 8: Avoid reapiting elements. Check out my previous answers for other project and you'll find out how to fix it.
<button className="button--footer" onClick={showAll}>
All
</button>
<button className="button--footer" onClick={showActive}>
Active
</button>
<button className="button--footer" onClick={showCompleted}>
Completed
</button>
9: Bad practice to attach events to wrong elements. Yes, it would work. But your <label /> isn't a button. Why do you handle click on it?
<input type="checkbox" className="checkbox" />
<label
onClick={handleClick}
htmlFor="checkbox"
className={`checkbox-label ${
currentTheme === "light"
? "checkbox-label--light"
: "checkbox-label--dark"
} ${isCompleted ? "label-checked" : ""}`}
>
Change your code to this and it would be better:
<input type="checkbox" id={`checkbox-${id}`} className="checkbox" onClick={handleClick} />
<label
htmlFor={`checkbox-${id}`}
className={`checkbox-label ${
currentTheme === 'light' ? 'checkbox-label--light' : 'checkbox-label--dark'
} ${isCompleted ? 'label-checked' : ''}`}
>
To handle click on label you need the same id on label and input. 10: Your <Todo /> doesn't need to know about all todos. So, we can change it. Remove some props and move handlers to parent component.
<Todo
todo={currentTodo}
currentTheme={currentTheme}
handleDelete={handleDelete}
handleClick={handleClick}
/>
TodoList.js
const handleDelete = (id) => {
const filtered = allTodos.filter((todo) => todo.id !== id)
setAllTodos(filtered)
setFilteredTodos(filtered)
}
const handleClick = (id) => {
const updatedArray = allTodos.map((currentTodo) => {
if (currentTodo.id === id) {
return {
...currentTodo,
isCompleted: !currentTodo.isCompleted,
}
}
return currentTodo
})
setAllTodos(updatedArray)
setFilteredTodos(updatedArray)
}
Now <ToDo /> is:
export default function Todo({ todo, currentTheme, handleDelete, handleClick }) {
return (
<>
<input
type="checkbox"
id={`checkbox-${todo.id}`}
className="checkbox"
onClick={() => handleClick(todo.id)}
/>
<label
htmlFor={`checkbox-${todo.id}`}
className={`checkbox-label ${
currentTheme === 'light' ? 'checkbox-label--light' : 'checkbox-label--dark'
} ${todo.isCompleted ? 'label-checked' : ''}`}
>
<svg className="check" xmlns="http://www.w3.org/2000/svg" width="11" height="9">
<path
fill="none"
className={`${currentTheme === 'light' ? 'check--light' : 'check--dark'} ${
todo.isCompleted ? 'label-checked' : ''
}`}
strokeWidth="2"
d="M1 4.304L3.696 7l6-6"
/>
</svg>
</label>
<span>{todo.description}</span>
<svg
className="cross"
onClick={() => handleDelete(todo.id)}
xmlns="http://www.w3.org/2000/svg"
width="18"
height="18"
>
<path
fill="#494C6B"
fillRule="evenodd"
d="M16.97 0l.708.707L9.546 8.84l8.132 8.132-.707.707-8.132-8.132-8.132 8.132L0 16.97l8.132-8.132L0 .707.707 0 8.84 8.132 16.971 0z"
/>
</svg>
</>
)
}
Figure this out. If have questions - feel free to ask.
Marked as helpful
@Hemazyn
Submitted
Hello everyone!
I'm excited to share that I've completed the frontend mentor project “news-hompage-ui," and I'm reaching out to the community for your valuable feedback. Your insights will be immensely helpful in refining and improving my work. Here are some specific areas where I would love to hear your thoughts:
Additionally, I'm currently facing a challenge in implementing the design for the button of scores, as outlined in the initial design challenge. Any guidance or suggestions on successfully completing this specific aspect of the project would be incredibly helpful.
I underestimated the challenge and recognize that there is always room for improvement. Your feedback is crucial in helping me grow as a developer. Thank you all for your time and consideration. I'm genuinely looking forward to hearing your thoughts and recommendations!
Best regards
@jebbaited
Posted
Hey!
1: Avoid using repeatable code. You work with React. Create components and pass props inside. If you don't need Component for some part of your code you could declare an array and fill it with, for example, nav names. Then just map it and you will have less code and high readability. Here is example from your repository. You use the same code 5 times and then 5 times more, but for phone view. Try to change it using advice above.
<li className="md:px-6 text-sm md:text-sm cursor-pointer capitalize font-inter font-normal text-Dark-grayish-blue hover:text-Soft-red"
onClick={() => { setNav(!nav); closeNavOnLargeScreen() }}>Home</li>
<li className="md:px-6 text-sm md:text-sm cursor-pointer capitalize font-inter font-normal text-Dark-grayish-blue hover:text-Soft-red"
onClick={() => { setNav(!nav); closeNavOnLargeScreen() }}>New</li>
<li className="md:px-6 text-sm md:text-sm cursor-pointer capitalize font-inter font-normal text-Dark-grayish-blue hover:text-Soft-red"
onClick={() => { setNav(!nav); closeNavOnLargeScreen() }}>Popular</li>
<li className="md:px-6 text-sm md:text-sm cursor-pointer capitalize font-inter font-normal text-Dark-grayish-blue hover:text-Soft-red"
onClick={() => { setNav(!nav); closeNavOnLargeScreen() }}>Trending</li>
<li className="md:px-6 text-sm md:text-sm cursor-pointer capitalize font-inter font-normal text-Dark-grayish-blue hover:text-Soft-red"
onClick={() => { setNav(!nav); closeNavOnLargeScreen() }}>Categories</li>
Same propblem here:
{/* one */}
<div className="flex flex-col">
<h4 className="text text-Off-white hover:text-Soft-orange cursor-pointer mb-2">Hydrogen VS Electric Cars</h4>
<p className="text-Grayish-blue font-light text-sm">Will hydrogen-fueled cars ever catch up to EVs?</p>
</div>
<hr className="line my-3 lg:my-0" />
{/* two */}
<div className="flex flex-col">
<h4 className="text text-Off-white hover:text-Soft-orange cursor-pointer mb-2">The Downsides of AI Artistry</h4>
<p className="text-Grayish-blue font-light text-sm">What are the possible adverse effects of on-demand AI image generation?</p>
</div>
{/* three */}
<hr className="line my-3 lg:my-0" /
.
.
.
2: When you need to handle onClick event create a function with name handleOnClick. Do inside whatever you want to and then pass this function to button onClick. And also you repeat the same function several times in your code.
onClick={() => { setNav(!nav); closeNavOnLargeScreen() }}
onClick={() => { setNav(!nav); closeNavOnLargeScreen() }}
onClick={() => { setNav(!nav); closeNavOnLargeScreen() }}
3: Create Component for this part
return (
<div key={id} className="flex flex-row gap-5">
<img src={icon} alt={alt} loading="lazy" className="h-[150px]" />
<div className="flex flex-col gap-2 mt-2">
<h1 className="text-3xl font-bold text-Grayish-blue">{number}</h1>
<h3 className="text-Very-dark-blue font-semibold hover:text-Soft-red cursor-pointer lg:text-sm">{head}</h3>
<p className="text-sm lg:text-xs font-light text-Dark-grayish-blue">{body}</p>
</div>
</div>
)
Feel free to ask if you have questions.
Marked as helpful
@lukKotecki
Submitted
Some similar features are handled differently because I wanted to practice some skills. The most challenging part was to provide order of mathematical operations, for ex: 2+2*2=6 instead of 8. I handled this with an array of input because I also wanted to implement undo operation with 'DEL' button. It's clunky and heavy code but it works.
@jebbaited
Posted
If you don't mind I'll leave some comments to your code.
1: Use Prettier to format code. It is much easier to read when formatted. Even with default settings.
2: Use error-first approach if possible when writing conditions. Your code:
function removeLastInput(){
if(input.length){
setInput(input.filter((el,i)=> input.length-1 !==i && el ))
}
}
My code:
function removeLastInput() {
if (!input.length) return
setInput(input.filter((el, i) => input.length - 1 !== i && el))
}
function replaceLastInput(value) {
if (!input.length) return
setInput(input.map((el, i) => (input.length - 1 !== i ? el : value)))
}
3: In switch/case construction you can use return instead of break. Less rows in general.
if (e.target.value) {
switch (value) {
case 'RESET':
return setInput([])
case 'DEL':
return removeLastInput()
case '=':
return calculateResult(concatNumbers())
default:
setCharToInput(value)
}
}
4: Variable creation. You could create an array in another way. Your code:
const firstArray = [...concatedNumbers]
const secondArray = []
.
.
.
firstArray.forEach((el) => el && secondArray.push(el))
My code:
const firstArray = [...concatedNumbers]
.
.
.
const secondArray = firstArray.map((el) => el)
Array.map() returns a new array and you don't need to push to another which you created above.
5: Some conditions difficult to understand and maintain. Like this one:
if (
previousInput === value ||
!previousInput ||
previousInput === '.' ||
isNaN(input[0] && isNaN(input[1])) ||
(isNaN(previousInput) && isNaN(input[input.length - 2]) && input.length >= 2) ||
(input.length === 1 && previousInput === '-')
)
Should be a good solution to create couple variables with good naming and then put them into this condition.
const notNaN = isNaN(input[0] && isNaN(input[1]))
const someName = isNaN(previousInput) && isNaN(input[input.length - 2]) && input.length >= 2
if (
previousInput === value ||
!previousInput ||
previousInput === '.' ||
notNaN ||
someName ||
(input.length === 1 && previousInput === '-')
)
6: Try to not repeat similar components:
<Button>7</Button>
<Button>8</Button>
<Button>9</Button>
<Button className="del">DEL</Button>
<Button>4</Button>
<Button>5</Button>
Create an array and then go throught it.
const Buttons = [
{
name: '7',
},
{
name: '8',
},
{
name: '9',
},
{
name: 'DEL',
class: 'del',
},
{
name: '4',
},
{
name: '5',
},
{
name: '6',
},
{
name: '+',
},
{
name: '1',
},
{
name: '2',
},
{
name: '3',
},
{
name: '-',
},
{
name: '.',
},
{
name: '0',
},
{
name: '/',
},
{
name: 'x',
},
{
name: 'RESET',
class: 'reset',
},
{
name: '=',
class: 'equal',
},
]
<Keyboard onClick={handleClick}>
{Buttons.map((button) => (
<Button className={button.class ? button.class : ''}>{button.name}</Button>
))}
</Keyboard>
Also you don't need <Keyboard /> component. Just handle clicks on every button.
<div className="kayboard">
{Buttons.map((button, index) => (
<Button className={button.class ? button.class : ''} onClick={handleClick} key={index}>
{button.name}
</Button>
))}
</div>
7: Let's change <Button/> component a little bit.
export default function Button({ name, className, onClick }) {
return (
<button value={name} className={`button ${className}`} onClick={onClick}>
{name}
</button>
)
}
Now you can use without children props. Cause why do you need it if you pass only button name.
<Button
className={button.class ? button.class : ''}
onClick={handleClick}
name={button.name}
key={index}
/>
8: ThreeStateToggle. Here we'll also avoid using same <input /> couple times.
Create <Input /> component:
const Input = forwardRef(function Input({ number, id, handleClick }, ref) {
return (
<>
<span>{number}</span>
<input
ref={ref}
onClick={handleClick}
className="toggle"
type="radio"
name="toggle"
id={id}
/>
</>
)
})
Then use it:
const themeSwitchInputs = [
{ number: 1, id: 'dark', ref: dark },
{ number: 2, id: 'light', ref: light },
{ number: 3, id: 'violet', ref: violet },
]
.
.
.
<div className="tri-state-toggle">
{themeSwitchInputs.map((input) => (
<Input
number={input.number}
id={input.id}
handleClick={handleClick}
ref={input.ref}
key={input.number}
/>
))}
</div>
About forwardRef read DOCS
Marked as helpful