Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Develop #1051

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

JulyaPetrovskaya
Copy link

@TarasHoliuk
Copy link

TarasHoliuk commented Sep 30, 2024

Fix lint error and tests:

image

image

Opened todo should have different icon:
image

Feel free to ask for help in the chat if you need 🙂

Copy link

@TarasHoliuk TarasHoliuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GJ 👍

Added a few small comments

src/App.tsx Outdated

setTodos(todos);
} catch (error) {
console.error('Error loading todos:', error);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disable lint rule or handle error in different way

src/App.tsx Outdated
useEffect(() => {
const fetchTodos = async () => {
try {
const todos = await getTodos();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const todos = await getTodos();
const fetchedTodos = await getTodos();

to avoid lint error

src/App.tsx Outdated
Comment on lines 15 to 16
const searchQuery = useSelector((state: RootState) => state.filter.query);
const status = useSelector((state: RootState) => state.filter.status);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const searchQuery = useSelector((state: RootState) => state.filter.query);
const status = useSelector((state: RootState) => state.filter.status);
const { searchQuery, status } = useSelector((state: RootState) => state.filter);

src/App.tsx Outdated
fetchTodos();
}, []);

const filteredTodos = todos.filter(todo => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use useMemo

return matchesQuery && matchesStatus;
});

const handleTodoClick = (todo: Todo) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant function wrapper, just use setSelectedTodo everywhere instead

reducers: {},
reducers: {
setSearchQuery: (state, action) => {
state.query = action.payload;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disable this rule for the entire file

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants