Develop #2596
Develop #2596
Conversation
| const filteredTodos = (() => { | ||
| const newTodos = todos.filter(todo => { | ||
| const matchesFilter = (() => { | ||
| switch (filterOption) { | ||
| case TodoState.completed: | ||
| return todo.completed; | ||
| case TodoState.active: | ||
| return !todo.completed; | ||
| default: | ||
| return true; | ||
| } | ||
| })(); | ||
|
|
||
| const matchesQuery = todo.title | ||
| .toLocaleLowerCase() | ||
| .includes(query.toLocaleLowerCase()); | ||
|
|
||
| return matchesFilter && matchesQuery; | ||
| }); | ||
|
|
||
| return newTodos; | ||
| })(); |
There was a problem hiding this comment.
You can create folder "utils" and move this function there
| className="input" | ||
| placeholder="Search..." | ||
| value={query} | ||
| onChange={event => setQuery(event.target.value.trimStart())} |
There was a problem hiding this comment.
You don't need trim this. In example, we can write with space, and this change result
| ); | ||
| <tbody> | ||
| {todos.map(todo => ( | ||
| <tr key={todo.id} data-cy="todo" className=""> |
There was a problem hiding this comment.
The key is generated on render, which may lead to performance issues. It's better to use a unique and consistent key, like todo.id.
| data-cy="selectButton" | ||
| className="button" | ||
| type="button" | ||
| onClick={() => setActiveTodo(todo)} |
There was a problem hiding this comment.
The naming conventions for event handlers are not followed. It is recommended to use the handle prefix for methods that are used as event handlers. In this case, the method could be named handleSetActiveTodo.
| > | ||
| <span className="icon"> | ||
| <i | ||
| className={!activeTodo ? 'far fa-eye' : 'far fa-eye-slash'} |
There was a problem hiding this comment.
The classnames library can be used here to conditionally set the class for this element. This will make the code cleaner and easier to read.
| getUser(activeTodo.userId) | ||
| .then(setTodoUser) | ||
| .finally(() => setIsLoading(false)); | ||
| }, []); |
There was a problem hiding this comment.
You have an empty array as a dependency for your useEffect hook. This means that the effect will only run once after the initial render. However, you're using activeTodo.userId inside the effect, which is a prop. If activeTodo changes, your effect won't run again as expected. You should include activeTodo.userId in the dependency array to ensure that the effect runs whenever activeTodo.userId changes.
| type="button" | ||
| className="delete" | ||
| data-cy="modal-close" | ||
| onClick={() => setActiveTodo(null)} |
There was a problem hiding this comment.
You're directly manipulating the DOM by setting setActiveTodo to null in your onClick event handler. In React, it's better to avoid direct DOM manipulation as much as possible. Consider using a more React-like approach, such as setting state or using context.
|
|
||
| {' by '} | ||
|
|
||
| <a href="mailto:[email protected]">Leanne Graham</a> | ||
| {todoUser && ( | ||
| <a href={`mailto:${todoUser.email}`}>{todoUser.name}</a> |
There was a problem hiding this comment.
You're generating a new function on each render by using an inline function for the href attribute. It's better to avoid generating new functions during render because it can lead to unnecessary re-renders and negatively impact performance. Consider moving this function outside of the component or memoizing it with useCallback.
| @@ -15,25 +32,35 @@ | |||
| className="modal-card-title has-text-weight-medium" | |||
| data-cy="modal-header" | |||
| > | |||
There was a problem hiding this comment.
You're generating a new string on each render by using a template literal for the Todo #${activeTodo.id} text. It's better to avoid generating new values during render because it can lead to unnecessary re-renders and negatively impact performance. Consider moving this value outside of the component or memoizing it with useMemo.
DEMO LINK