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 #780

Closed
wants to merge 7 commits into from
Closed

Develop #780

wants to merge 7 commits into from

Conversation

AndrMar1939
Copy link

Copy link

@BudnikOleksii BudnikOleksii left a comment

Choose a reason for hiding this comment

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

Looks good 🚀
I Left some comments to improve your solution, check them.
Also, a lot of tests failed:
image

import { TodosProvider } from './Context';
import USER_ID from './helpers/USER_ID';

// components

Choose a reason for hiding this comment

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

Remove comments

Comment on lines +16 to +23
const { setIsFocused } = useContext(FormFocusContext);
const {
todos,
filter,
setFilter,
dispatch,
} = useContext(TodosContext);
const { setApiError } = useContext(ApiErrorContext);

Choose a reason for hiding this comment

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

Create hooks for your contexts, so you don't need to pass a context every time.
As example:

const useTodos = () => useContext(TodosContext);

Comment on lines 27 to 37
<input
data-cy={forCypress}
type="text"
className={className}
placeholder={placeholder}
onChange={onInputChange}
ref={ref}
value={value}
onBlur={onBlur}
onKeyUp={onKeyUp}
/>

Choose a reason for hiding this comment

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

Hint:

  • if you don't need at this level props and just want to pass them to the next component you can use the spread syntax. Example:
function Profile(props) {
  return (
    <div className="card">
      <Avatar {...props} />
    </div>
  );
}

https://react.dev/learn/passing-props-to-a-component

  • also, in this case, you can reuse props type

In your case get onSubmit from props and pass other props to the next component

import { emptyInputError } from '../../types/apiErrorsType';
import { getActiveTodos } from '../../helpers/getTodos';

// Component

Choose a reason for hiding this comment

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

Don't leave redundant comments, remove all of them

Comment on lines +97 to +98
patchTodo(id, data)
.then((patchedTodo) => {

Choose a reason for hiding this comment

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

Don't do async operations in forEach. You can use await in for of loop or even better with map collect all your requests and use Promise.all

Comment on lines +20 to +37
type TodosContextType = {
todos: TodosListType,
dispatch: React.Dispatch<Actions>,
filter: FiltersType,
setFilter: React.Dispatch<React.SetStateAction<FiltersType>>
tempTodo: Todo | null,
setTempTodo: React.Dispatch<React.SetStateAction<Todo | null>>
};

type ApiErrorContextType = {
apiError: ApiErrorType,
setApiError: React.Dispatch<React.SetStateAction<ApiErrorType>>
};

type FormFocusContextType = {
isFocused: boolean,
setIsFocused: React.Dispatch<React.SetStateAction<boolean>>
};

Choose a reason for hiding this comment

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

Create a separate file for each context so it will be easy to maintain them

Comment on lines +94 to +100
<FormFocusContext.Provider value={{ isFocused, setIsFocused }}>
<ApiErrorContext.Provider value={{ apiError, setApiError }}>
<TodosContext.Provider value={todosContextValue}>
{children}
</TodosContext.Provider>
</ApiErrorContext.Provider>
</FormFocusContext.Provider>

Choose a reason for hiding this comment

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

Split contexts, you can wrap like this in main component

Comment on lines 22 to 23
default:
return todos;

Choose a reason for hiding this comment

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

Suggested change
default:
return todos;
case FiltersType.ALL:
default:
return todos;

You can combine cases

Comment on lines +3 to +24
export type Actions = {
type: 'LOAD',
payload: TodosListType,
} | {
type: 'POST',
payload: Todo,
} | {
type: 'DELETE',
payload: number,
} | {
type: 'PATCH',
payload: Todo,
} | {
type: 'ALL_ACTIVE',
payload: null,
} | {
type: 'IS_SPINNING',
payload: number,
} | {
type: 'REMOVE_SPINNING',
payload: number,
};

Choose a reason for hiding this comment

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

It's not readable, create a type for each action and then combine them with union type

@@ -0,0 +1,5 @@
export type EmptyInputErrorType = 'REQUIRED';

export const emptyInputError: EmptyInputErrorType = 'REQUIRED';

Choose a reason for hiding this comment

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

It's too much, if you set some string it gets only this string as type so you don't need to create another type for constant

@AndrMar1939
Copy link
Author

AndrMar1939 commented Sep 22, 2023

Looks good 🚀 I Left some comments to improve your solution, check them. Also, a lot of tests failed: image

Yep!!! I was trying to work with Bulma's additional styles, so the editing tests failed. Okay.

Copy link

@Moroz-Dmytro Moroz-Dmytro left a comment

Choose a reason for hiding this comment

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

I see Oleksii has already reviewed it
Just some bug with the platform, I was not supposed to get this PR to review as it was not fixed yet
Need to reject this 🙂
Please, fix previous comments and I'll review it 😉

@AndrMar1939 AndrMar1939 closed this by deleting the head repository Sep 25, 2023
@AndrMar1939 AndrMar1939 mentioned this pull request Sep 26, 2023
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