31Vector31 / react-training

Hooks, Axios, HOC, Redux, Context, Reselect, Router.
0 stars 0 forks source link

review phone-book-redux #12

Closed shevchuknine closed 2 years ago

shevchuknine commented 2 years ago

https://github.com/31Vector31/react-training/blob/0b95f63863a5b87c6c2f605862d449e41baba6c7/src/PhoneBookRedux/components/PhoneBook/PhoneBook.js#L9-L16 инлайновые стили в самую последнюю очередь. динамически добавляешь класс и описываешь его глобально в стилях https://github.com/31Vector31/react-training/blob/0b95f63863a5b87c6c2f605862d449e41baba6c7/src/PhoneBookRedux/actions/main.js#L14 обычные экнешы должны быть строго чистыми функциями, в твоем случае необходимо использовать асинхронный экшн в случае, когда ты используешь асинхронные экшены, имеет смысл относиться к редьюсеру как к сущности, которая просто устанавливает значение в стор, а всю логику в себе реализуют асинхронные экшены. у тебя есть отдельные экшены, которые работают с формой (утсановка значение и валидация) - они вызываются всегда вместе, но все-равно разделены на отдельные синхронные экшены. можно их объеденить и прикрутить валидацию к ним же, чтобы исзбавить от этого редьюсер https://github.com/31Vector31/react-training/blob/0b95f63863a5b87c6c2f605862d449e41baba6c7/src/PhoneBookRedux/containers/ContactInfo.js#L7 ты дублируешь этот код в нескольких компонентах - можно вынести в селектор и переиспользовать https://github.com/31Vector31/react-training/blob/0b95f63863a5b87c6c2f605862d449e41baba6c7/src/PhoneBookRedux/containers/PopupConfirm.js#L12-L16 яркий признак того, что все это можно объеденить в один асинхронный экшен _ разделенные редьюсеры имеют общие черты, можно объеденить, например, ревьюсер по работе с контактами и который устанавливает выбранного контакта - они ведь работают с одной сущностью. то же и с формой (установка значения и валидация)

shevchuknine commented 2 years ago

https://github.com/31Vector31/react-training/blob/5730f7535507bda9d08304b9a1829e07fe89c934/src/PhoneBookRedux/actions/popupForm.js#L2-L9 у тебя изменение каждого поля неизменно сопровождается валидацией этого поля, почему бы не объеденить это в один экшен? https://github.com/31Vector31/react-training/blob/5730f7535507bda9d08304b9a1829e07fe89c934/src/PhoneBookRedux/reducers/idSelectedContact.js#L4 отдельный редьюсер, который просто хранит число - слишком избыточное решение. объедени этот редьюсер с тем, который отвечает за контакты https://github.com/31Vector31/react-training/blob/5730f7535507bda9d08304b9a1829e07fe89c934/src/PhoneBookRedux/reducers/visibilityPopupForm.js#L1 то же и здесь, этот редьюсер всегда будет работать вместе с тем, который хранит значения формы - можно их объеденить https://github.com/31Vector31/react-training/blob/5730f7535507bda9d08304b9a1829e07fe89c934/src/PhoneBookRedux/containers/PopupForm.js#L8 выглядит костылем. в случае, если нужно менять какой-либо контакт, ты заполняешь существующими данными поля формы и потом форму отправляешь в компонент https://github.com/31Vector31/react-training/blob/5730f7535507bda9d08304b9a1829e07fe89c934/src/PhoneBookRedux/components/PopupForm/PopupForm.js#L23 либо хуки, либо connect wrapper - не смешивать https://github.com/31Vector31/react-training/blob/5730f7535507bda9d08304b9a1829e07fe89c934/src/PhoneBookRedux/components/PopupForm/PopupForm.js#L40-L45 компонент должен знат ькак можно меньше про логику и состояние, потому эта логика (и подобная), должны быть вынесена в отдельный экшен и там обработана, включая hideClick. компонент должен понимать где произошел клик и что вызвать после этого клика, а вот уже реализация того, что будет вызвано должно быть неизвестно компоненту

shevchuknine commented 2 years ago

https://github.com/31Vector31/react-training/blob/4913046a4de200c5a66326d1730047399b1aa657/src/PhoneBookRedux/actions/popupForm.js#L3-L16 можно сгрупировать данные экшенов в payload, и тогда в редьюсере будет удобнее разбирать https://github.com/31Vector31/react-training/blob/4913046a4de200c5a66326d1730047399b1aa657/src/PhoneBookRedux/components/PopupForm/PopupForm.js#L14-L18 дефолтными значениями имеет смысл заполнять форму при открытии попапа в экшене https://github.com/31Vector31/react-training/blob/4913046a4de200c5a66326d1730047399b1aa657/src/PhoneBookRedux/components/PopupForm/PopupForm.js#L43-L49 заверни в доп. компонент, у тебя некоторые пропсы дублируются

shevchuknine commented 2 years ago

дефолтными значениями имеет смысл заполнять форму при открытии попапа в экшене

имеется в виду, что компонент не должен самостоятельно вызывать свою же инициализацию. это должно проихводиться в экшене, отдельно от компонента. для этого можно объединить экшены выбора контакта, открытия попапа и заполнения данных в асинхронный