solution #2615
solution #2615
Conversation
imondok03
commented
Oct 18, 2024
imondok03
commented
- DEMO LINK
There was a problem hiding this comment.
Great job on the pull request! 🎉 While there are some suggestions for improvements, they are mostly related to naming conventions and best practices that don't critically impact functionality. Keep up the good work and consider the feedback for future enhancements!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| const [isLoaded, setIsLoaded] = useState(false); | ||
| const [selectedTodo, setSelectedTodo] = useState<Todo | undefined>(undefined); | ||
| const [selectedStatus, setSelectedStatus] = useState('all'); | ||
| const [query, setSelectedQuery] = useState(''); |
There was a problem hiding this comment.
The state setter function here is named setSelectedQuery, but it should be setQuery to match the state variable name query. This follows the naming convention for state setter functions.
| setTodos(filteredResult); | ||
| setIsLoaded(true); | ||
| }); | ||
| }, [selectedStatus, query]); |
There was a problem hiding this comment.
Using props to initialize state directly can lead to unexpected behavior. Consider using a function inside useState or handling it differently to avoid potential issues.
| @@ -17,18 +46,30 @@ export const App: React.FC = () => { | |||
| <h1 className="title">Todos:</h1> | |||
|
|
|||
| <div className="block"> | |||
| <TodoFilter /> | |||
| <TodoFilter | |||
| onSelect={setSelectedStatus} | |||
There was a problem hiding this comment.
The naming convention for event handler props suggests using onStatusSelect instead of onSelect to make it more descriptive and specific to the action.
| <TodoFilter /> | ||
| <TodoFilter | ||
| onSelect={setSelectedStatus} | ||
| onChange={setSelectedQuery} |
There was a problem hiding this comment.
The naming convention for event handler props suggests using onQueryChange instead of onChange to make it more descriptive and specific to the action.
| <TodoList | ||
| todos={todos} | ||
| selectedTodo={selectedTodo} | ||
| onSelect={setSelectedTodo} |
There was a problem hiding this comment.
The naming convention for event handler props suggests using onTodoSelect instead of onSelect to make it more descriptive and specific to the action.
| <tr | ||
| data-cy="todo" | ||
| className={`${id === selectedId && 'has-background-info-light'}`} | ||
| key={id} |
There was a problem hiding this comment.
Generating keys directly from the id prop is generally fine if id is unique and stable. However, ensure that id is indeed unique across all items to avoid potential issues.
| const onEscape = (event: KeyboardEvent) => { | ||
| if (event.key === 'Escape') { | ||
| onClose(undefined); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The naming convention for event handler functions should follow the pattern handleEventName. Consider renaming onEscape to handleEscape to align with the recommended naming conventions.
| document.addEventListener('keydown', onEscape); | ||
| }); |
There was a problem hiding this comment.
Directly interacting with the DOM using document.addEventListener is not recommended in React. Consider using a cleanup function in useEffect to remove the event listener when the component unmounts.
| setLoader(true); | ||
| }); | ||
| document.addEventListener('keydown', onEscape); | ||
| }); |
There was a problem hiding this comment.
The useEffect hook should have a dependency array to avoid running on every render. Consider adding userId as a dependency to ensure the effect only runs when userId changes.
| type="button" | ||
| className="delete" | ||
| data-cy="modal-close" | ||
| onClick={() => onClose(undefined)} |
There was a problem hiding this comment.
The naming convention for event handler functions should follow the pattern handleEventName. Consider renaming the inline function to handleClose to align with the recommended naming conventions.
