sprint-2/step-2 #4
Conversation
There was a problem hiding this comment.
добрый день, отличная работа
- все работает как ожидается
- круто что используется redux toolkit и методология ducks для описания хранилища
- реализовано переключение навигации ингредиентов
но нужно немного исправить
- все необходимые поля должны быть помечены isRequired, тут же должно быть обязательно либо null либо ингредиент
- если при запросе к апи произошла ошибка, необходимо сбрасывать хранилище к начальному состоянию, то есть orderNumber должен стать null а ingredientsList должен стать пустым массивом
- хуки должны использовать только на верхнем уровне компонента и не могут быть обернуты в другие функции или условия
и можно сделать еще лучше
- имеет смысл отказаться от этого hoc withModal, потому что
- для открытия модалки используется несколько флагов в сторе: modalType и order.orderNumber для модалки заказа
- при добавлении каждой новой модалки ее контент придется добавлять в этот hoc
- лучше переложить request в utils потому что запросы не относятся к редаксу и хранению состояния
- лучше назвать duck ingredient и селектор - current-ingredient и getCurrentIngredient соответственно. чтобы по названию было более понятно для чего они используются
работа может быть принята только после исправления всех критических замечаний помеченных нужно исправить
удачи
src/services/api.js
Outdated
| @@ -0,0 +1,18 @@ | |||
| export const ENDPOINT = 'https://norma.nomoreparties.space/api'; | |||
|
|
|||
| const request = async (url, options = null) => { | |||
There was a problem hiding this comment.
круто что запросы к апи вынесены в отдельный модуль, но лучше переложить это в utils потому что запросы не относятся к редаксу и хранению состояния
| // Selectors | ||
| export const getAppState = ({ app }) => app; | ||
|
|
||
| export default appReducer; |
There was a problem hiding this comment.
круто что используется методология ducks для описания стора redux 👍
src/services/ducks/app.js
Outdated
| }) | ||
| .addCase(openModal, (state, { payload }) => { | ||
| state.isModalOpen = true; | ||
| state.modalType = payload; |
There was a problem hiding this comment.
можно сделать лучше: лучше не хранить состояние модальных окон отдельно, потому что в приложении уже есть флаги по которым можно показывать их. для модалки заказа это orderNumber в редьюсере order. для модалки ингредиента это ingredient в редьюсере ingredient. в текущей реализации получается что для открытия каждой модалки имеются три разных флага в хранилище
| ingredient: pt.oneOfType([ | ||
| PropValidator.INGREDIENT.isRequired, | ||
| pt.oneOf([null]).isRequired, | ||
| ]), |
There was a problem hiding this comment.
нужно исправить: все необходимые поля должны быть помечены isRequired, тут же должно быть обязательно либо null либо ингредиент
There was a problem hiding this comment.
Если везде поставить isRequired или только после oneOfType, то это не работает. Проблема вроде как распространенная и вообще это вроде баг. Если придет null в компонент, то будет ошибка "Warning: Failed prop type: The prop ingredient is marked as required in BunIngredient, but its value is null." , хотя стоит проверка на null. В моем же варианте при передаче любого другого типа будет ошибка "Warning: Failed prop type: Invalid prop ingredient supplied to BunIngredient."
facebook/prop-types#90 (comment)
There was a problem hiding this comment.
да, посмотрел, действительно так. пусть остается как есть
| export const getBurgerIngredients = ({ burgerIngredients }) => | ||
| burgerIngredients.burgerData; | ||
|
|
||
| export const getTotalPrice = createSelector( |
There was a problem hiding this comment.
отлично что подсчет стоимости бургера вынесен в селекторы 👍 а не находится в редьюсере к примеру
src/services/ducks/ingredient.js
Outdated
| }); | ||
|
|
||
| // Selectors | ||
| export const getIngredient = ({ ingredient }) => ingredient; |
There was a problem hiding this comment.
можно сделать лучше: лучше назвать этот duck и селектор current-ingredient и getCurrentIngredient соответственно. чтобы по названию было более понятно для чего они используются
src/services/ducks/order.js
Outdated
| state.isLoading = false; | ||
| }) | ||
| .addCase(sendOrder.rejected, (state) => { | ||
| state.isLoading = false; |
There was a problem hiding this comment.
нужно исправить: если при запросе к апи произошла ошибка, необходимо сбрасывать хранилище к начальному состоянию, то есть orderNumber должен стать null
| state.isLoading = false; | ||
| }) | ||
| .addCase(fetchAllIngredients.rejected, (state) => { | ||
| state.isLoading = false; |
There was a problem hiding this comment.
нужно исправить: если при запросе к апи произошла ошибка, необходимо сбрасывать хранилище к начальному состоянию, то есть ingredientsList должен стать пустым массивом
| const containerRef = useRef(); | ||
| const ingredientsTitleRef = useMemo(() => { | ||
| return TAB_ITEMS.reduce((acc, { type }) => { | ||
| acc[type] = createRef(null); |
There was a problem hiding this comment.
нужно исправить: хуки должны использовать только на верхнем уровне компонента и не могут быть обернуты в другие функции или условия
There was a problem hiding this comment.
CreateRef не хук. У меня стоит плагин валидации правил хуков в IDE и ошибок нет.
There was a problem hiding this comment.
Здесь вообще можно получить ответ???
There was a problem hiding this comment.
сорри за долгий ответ. я перепутал createRef и useRef. обычно правда createRef ипользуется в классах, а useRef в функциях. но поскольку в данной реализации есть мемоизация и createRef дергается единожды при mount, пусть остается как есть с createRef
src/components/hocs/with-modal.js
Outdated
|
|
||
| const getComponent = () => { | ||
| switch (modalType) { | ||
| case ModalType.ORDER: | ||
| return <OrderDetails orderNumber={orderNumber} />; | ||
| return <OrderDetails />; |
There was a problem hiding this comment.
можно сделать лучше: имеет смысл отказаться от этого hoc, потому что
- для открытия модалки используется несколько флагов в сторе: modalType и order.orderNumber для модалки заказа
- при добавлении каждой новой модалки ее контент придется добавлять в этот hoc
лучше использовать компонент Modal по месту, к примеру
const orderNumber = useSelector(getOrderNumber);
const dispatch = useDispatch();
...
return (
...
{orderNumber && (
<Modal closeModal={() => dispatch(resetOrder)}>
<OrderDetails number={orderNumber} />
</Modal>
)}
...
)
No description provided.