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

solution #1057

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

solution #1057

wants to merge 2 commits into from

Conversation

syavaYki
Copy link

@syavaYki syavaYki commented Oct 3, 2024

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.

Try to fix tests

@@ -0,0 +1,28 @@
import { Todo } from '../types/Todo';

export const LocalStorage = {

Choose a reason for hiding this comment

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

Suggested change
export const LocalStorage = {
export const localStorage = {

We use PascalCase for components, types, classes only


useEffect(() => {
dispatch({ type: 'getTodos' });
dispatch({ type: 'getFilteredTodos' });

Choose a reason for hiding this comment

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

it's a bit uncommon and better to remove filteredTodos from the reducer. General rule - no need to store value in the state if it can be computed from other state values. Compute filtered todos in place you need it based on todos and filter

So you will get rid of dispatching this action here

children: React.ReactNode;
};

export const GlobalProvider: React.FC<Props> = ({ children }) => {

Choose a reason for hiding this comment

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

it's also component, so it should be in components folder

};

const handleKeyEvent = (event: React.KeyboardEvent<HTMLInputElement>) => {
if (event.type === 'keyup') {

Choose a reason for hiding this comment

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

you add it only to this listener - redundant check

type Props = {
todo: Todo;
};
type SubmitProps =

Choose a reason for hiding this comment

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

Use correct event type in each case or just use SyntheticEvent

event.preventDefault();
setEditing(false);

if (newTitle.length === 0) {

Choose a reason for hiding this comment

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

Suggested change
if (newTitle.length === 0) {
if (!newTitle.length) {

shorter form
Alos, trim value to avoid adding spaces only


return (
<>
{todos.length > 0 && (

Choose a reason for hiding this comment

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

makes sense to add if and return null in this case
And here get rid of check and React.fragment

@syavaYki syavaYki requested a review from TarasHoliuk October 4, 2024 16:17
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