Skip to content

done sprint 12#3

Open
ArsPro1323 wants to merge 3 commits into
mainfrom
sprint-12
Open

done sprint 12#3
ArsPro1323 wants to merge 3 commits into
mainfrom
sprint-12

Conversation

@ArsPro1323

Copy link
Copy Markdown
Owner

No description provided.

@andryxxa93 andryxxa93 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Здравствуйте! Проделана большая работа 👍

Сильные стороны работы:

  • Правильно использовали redux-toolkit
  • Типизировали хранилище
  • Активная категория ингредиентов меняется корректно

Однако есть пару моментов, которые необходимо исправить, чтобы работа была принята.

Необходимо исправить:

  • Для всех запросов должен использоваться усилитель, где будет три типа экшенов -
    _REQUEST , тип _SUCCESS , _ERROR. Вы можете использовать createAsyncThunk из redux-toolkit.
  • использовать key={ item._id + index } для элементов конструктора нельзя - это может привести к неправильной работе dnd. Для атрибута key в конструкторе нужно использовать для ингредиента уникальный uuid (Вам нужно его генерировать во время сброса в зону DnD). Это можно делать с помощью библиотеки uuid https://www.npmjs.com/package/uuid , или Вам должны были показать, как это сделать, иначе некорректно может работать DND. А в key будет уже просто key={ item.uuid }

В pull request вы так же найдете комментарии с рекомендациями как можно улучшить код. Их делать не обязательно, однако в будущем они могут помочь избежать ошибок и сделать код лучше.

**Напомню, чтобы работа была принята нужно исправить все замечания помеченные как необходимо исправить.

Пожалуйста, проверьте работоспособность проекта и наличие возможных ошибок в консоли браузера (кнопка F12) перед отправкой на ревью.

Удачных правок!



const dispatch = useDispatch();
const {elements, bun} = useSelector((state) => state.elementConstructor)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Можно лучше
Хорошая практика - передавать в useSelector не анонимную, а именованную функцию. Это позволит избежать лишних вызовов useSelector.
https://www.youtube.com/watch?v=SVG-x-4BQic

}

export function ConstructorBottomElement(props: ElementProps) {
export function ConstructorBottomElement(props) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Можно улучшить
Здесь и в коде других компонентов, лучше не писать каждый раз props, а использовать деструктуризацию, достав все необходимые данные из пропсов один раз. Это сделает код более понятным. https://learn.javascript.ru/destructuring#destrukturizatsiya-obekta

@ArsPro1323

Copy link
Copy Markdown
Owner Author

@andryxxa93
А точно такие типы экшенов? разве не LOADING вместо REQUEST?

@andryxxa93 andryxxa93 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Супер, вы отлично поработали!
Поздравляю! Работа принята.
Желаю удачного продолжения обучения!

@andryxxa93

Copy link
Copy Markdown

@andryxxa93 А точно такие типы экшенов? разве не LOADING вместо REQUEST?

Можно и loading, главное чтобы был экшен для обозначения загрузки

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