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

initial commit #765

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

initial commit #765

wants to merge 1 commit into from

Conversation

aliliolek
Copy link

edit: false,
});
const [currentFilter, setCurrentFilter] = useState(TodoFilter.All);
const [loadingId, setLoadingId] = useState<number[]>([]);

Choose a reason for hiding this comment

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

Suggested change
const [loadingId, setLoadingId] = useState<number[]>([]);
const [loadingTodoIds, setLoadingTodoIds] = useState<number[]>([]);

array - use plural form


export const App: React.FC = () => {
const [todos, setTodos] = useState<Todo[]>([]);
const [visibleTodos, setVisibleTodos] = useState<Todo[]>([]);
const [errors, setErrors] = useState({

Choose a reason for hiding this comment

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

use enum instead of object, where default value is Error.Empty:

enum Error {
  Empty = ''
  // other errors
}


const changeTodo = (
property: string,
// eslint-disable-next-line @typescript-eslint/no-explicit-any

Choose a reason for hiding this comment

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

why any? try to figure out with types (maybe boolean | string, etc)


if (newTitle.trim()) {
addTodo(newTitle);
} else if (newTitle.trim() === '') {

Choose a reason for hiding this comment

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

Suggested change
} else if (newTitle.trim() === '') {
} else {

/>

{/* show only one message at a time */}
{errors.load && (

Choose a reason for hiding this comment

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

create function which returns you the correct error message (using switch) or just save the correct message in the enum mentioned above

};

const saveChanges = () => {
if (inputValue.trim() && inputValue.trim() !== title.trim()) {

Choose a reason for hiding this comment

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

no need to trim title here, save inputValue.trim() to a variable and reuse

};

return (
<div className={`todo ${completed ? 'completed' : ''}`}>

Choose a reason for hiding this comment

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

use classnames library for such cases

const [isEditingId, setIsEditingId] = useState<number | null>(null);
const [inputValue, setInputValue] = useState(title);

const handleDoubleClick = () => {

Choose a reason for hiding this comment

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

you should also focus on input when it appears - use ref for it (ask in chat if you have questions)

Comment on lines +151 to +153
useEffect(() => {
filterTodos();
}, [currentFilter, todos]);

Choose a reason for hiding this comment

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

you don't need visibleTodos state
general rule if you calculate some value based on other state values/props no need to save it in state

save it in a common variable, wrap in useMemo with the same dependency array

Comment on lines +36 to +40
if (initTodos) {
filtered = [...initTodos];
} else {
filtered = [...todos];
}

Choose a reason for hiding this comment

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

you should get rid of it - make todos argument mandatory

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.

2 participants