Month7/step2 #2
Month7/step2 #2
Conversation
gennady-bars
left a comment
gennady-bars
left a comment
There was a problem hiding this comment.
Здравствуйте. (Нужно развернуть общий комментарий ↓)
Работа проделана огромная
- Отлично, что ловите возможные ошибки в конце каждого запроса к серверу
- Отлично, что используете пустой массив зависимостей
[], чтобыuseEffectсработал 1 раз - Отлично, что модальные окна в
App. Это поможет в будущем
но есть некоторые недочеты:
- Если переменная не перезаписывает своё значение, она объявляется через
const. - Любой ответ от сервера нужно проверять на
ok(на корректность) и вызывать ошибку, если ответ неok. Таким образом, вcatchВы будете ловить и ошибки сервера, и ошибки невалидных данных. - Сквозь оверлей должна быть видна главная страница. Скрин https://disk.yandex.ru/i/-0PKns6EJ_RvRw
- Попапы не должны закрываться при клике внутри них
- по заданию нужно называть папки и файлы в
kebab-case, а у Вас все вcamelCase. <>пустые теги нужны только тогда, когда нет общего тега (компонента), который оборачивает всю верстку
Исправьте, пожалуйста, недочеты и работа будет принята. Пожалуйста, проверьте работоспособность проекта и наличие возможных ошибок в консоли браузера (кнопка F12) перед отправкой на ревью.
Напоминаю, что работа может быть принята только после исправления всех критических замечаний Нужно исправить.
Удачного рефакторинга кода.
src/components/app/app.jsx
Outdated
| import {IngredientDetails} from "../ingredientDetails/ingredientDetail.jsx"; | ||
|
|
||
| function App() { | ||
| let [state, setState] = React.useState({ |
There was a problem hiding this comment.
Если переменная объявлена через let, её значение должно быть напрямую перезаписано.
Если переменная не перезаписывает своё значение, она объявляется через const.
Все переменные нужно объявлять по возможности через const.
В больших функциях бывает так, что Вы можете не заметить, если переменная переназначится, и это может оказаться причиной багов. Особенно в замыканиях.
const дает Вам уверенность в том, что Вы всегда будете видеть одно и то же значение.
src/components/app/app.jsx
Outdated
| try { | ||
| setState({ ...state, isLoading: true, hasError: false }); | ||
| fetch("https://norma.nomoreparties.space/api/ingredients") | ||
| .then((res) => res.json()) |
There was a problem hiding this comment.
.then((res) => res.json())
Любой ответ от сервера нужно проверять на ok (на корректность) и вызывать ошибку, если ответ не ok. Таким образом, в catch Вы будете ловить и ошибки сервера, и ошибки невалидных данных.
.then(res => {
if (res.ok) {
return res.json();
}
return Promise.reject(`Ошибка ${res.status}`);
})| .then((data) => { | ||
| setState({ ...state, dataBurger: data.data }); | ||
| }); | ||
| } catch (err) { |
There was a problem hiding this comment.
Отлично, что ловите возможные ошибки в конце каждого запроса к серверу
| } | ||
| }; | ||
| getBurgerData(); | ||
| }, []); |
There was a problem hiding this comment.
Отлично, что используете пустой массив зависимостей [], чтобы useEffect сработал 1 раз
| <BurgerIngredients openItem={openItem} data={state.dataBurger} /> | ||
| <BurgerConstructor data={state.dataBurger} openOrder={openOrder} /> | ||
| </main> | ||
| {order && <Modal closePopup={closePopup} title=""> |
There was a problem hiding this comment.
Отлично, что модальные окна в App, Это поможет в будущем
| @@ -0,0 +1,42 @@ | |||
| import React from "react"; | |||
| import styles from "./appHeader.module.css"; | |||
There was a problem hiding this comment.
- по заданию нужно называть папки и файлы в
kebab-case, а у Вас все вcamelCase.
| }; | ||
| document.addEventListener("keydown", closeHandler); | ||
| return () => { | ||
| document.removeEventListener("keydown", closeHandler); |
src/components/modal/modal.jsx
Outdated
| }, [closePopup]); | ||
|
|
||
| return createPortal( | ||
| <> |
There was a problem hiding this comment.
<> пустые теги нужны только тогда, когда нет общего тега (компонента), который оборачивает всю верстку
|
|
||
| return createPortal( | ||
| <> | ||
| <section> |
There was a problem hiding this comment.
<section> можно удалить, так как не имеет стилей. Это ничего не дает
если хотите секцию обозначить, то можно внутри оверлея его указать вместо div
но и секция не нужна для попапа. Это не секция чего-то по семантике
src/components/modal/modal.jsx
Outdated
| <> | ||
| <section> | ||
| <ModalOverlay closePopup={closePopup}> | ||
| <div className={styles.popupContainer}> |
There was a problem hiding this comment.
- Попапы не должны закрываться при клике внутри них
исправленные ошибки
gennady-bars
left a comment
gennady-bars
left a comment
There was a problem hiding this comment.
Поздравляю! Ваша работа принята.
Вы отлично потрудились.
Удачного дальнейшего обучения.
второй этап проекта