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

добавлен Context #3

Merged
merged 3 commits into from
Dec 10, 2022
Merged

добавлен Context #3

merged 3 commits into from
Dec 10, 2022

Conversation

Tahora
Copy link
Owner

@Tahora Tahora commented Nov 22, 2022

No description provided.

Copy link

@ArslanMustafin ArslanMustafin left a comment

Choose a reason for hiding this comment

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

Code Review

Здравствуйте!

Отличная работа.

Вы молодец!

  • Реализована работа с контекстом.

Но есть несколько замечаний, которые нужно исправить:

  • При отправке запроса на создание заказа POST /orders, в поле ingredients следует передавать обе булки, а не 1. Т.е. id булок должны быть в самом начале массива, и в конце, а также они должны быть одинаковы;

  • У вас неправильно считается цена, https://yadi.sk/d/DcrZkZh-DQVdUw. В каких-то случаев она считается нормально, а в каких-то нет.

  • По макету, в компоненте App-header нужно чтобы была активна только 1 ссылка.

  • Исправить ошибку в консоли: https://yadi.sk/d/JEjFT55o0jGmdw;

  • Структуру ингредиента лучше описать таким образом:

    export const ingredientPropType = PropTypes.shape({
      _id: PropTypes.string.isRequired,
      name: PropTypes.string.isRequired,
      type: PropTypes.oneOf(["bun", "main", "sauce"]).isRequired,
      proteins: PropTypes.number.isRequired,
      fat: PropTypes.number.isRequired,
      carbohydrates: PropTypes.number.isRequired,
      calories: PropTypes.number.isRequired,
      price: PropTypes.number.isRequired,
      image: PropTypes.string.isRequired,
      image_large: PropTypes.string.isRequired,
      image_mobile: PropTypes.string.isRequired,
    });

    А также необходимо указывать .isRequired как у элемента массива, так и у самого массива в PropTypes.
    Пример как правильно:

    // массив
    data: PropTypes.arrayOf(ingredientTypes.isRequired).isRequired;
    
    // одиночный элемент
    data: ingredientTypes.isRequired;

    И в описании propTypes компонентов нужно указывать .isRequired у пропсов (Если это требуется), без этого не контролируется, что пропс передается

    Проверьте во всем проекте.

Сделает код лучше:

  • лучше всегда использовать строгое сравнение === и строгое неравенство !==, это избавит от некоторых неявных ошибок связанных с приведением типов (например при использовании нестрогого сравнения выражение 0 == "" вернет true, что не всегда ожидаемо, а выражение 0 === "" вернет false т.к. типы у значений слева и справа разные, слева тип number, а справа тип string)

  • не стоит в обработчика событий использовать двойные функции. Если вам нужно передать какой-то параметр используйте анонимные функции.

    const handleClick = (param) => {
      /** ... */
    };
    
    <Component onClick={() => handleClick(param)} />;
  • при клике на таб скролить список ингредиентов к нужному разделу.

  • Перед отправкой работы убедитесь в том, что весь неиспользуемый код удален.

  • Стоит обращать внимание на предупреждения в терминале, в котором запущен проект. Это хорошие рекомендации, которых следовало бы придерживаться. В противном случае, лучше отключить линтер у строки (куска кода) eslint-disable. Так же добавить комментарий (TODO), объясняющий причину блокировки линтера в данном месте. Суть в том, что мы стараемся писать более чистый код, а если видим, что порой выскакивают предупреждения того же линтера, то оставляем комментарий, объясняющий причину. В коммерческой разработке, когда есть целая команда разработчиков, работающих над одним проектом, эта рекомендация послужит правилом хорошего тона. Неплохая статься по поводу локального отключения линтера: https://learn.coderslang.com/ru/0023-eslint-disable-for-specific-lines-files-and-folders

  • В некоторых местах нарушено форматирование кода, проблемы с отступами и в некоторых местах слишком длинные строки, что затрудняет чтение кода. Об оформлении кода можно почитать здесь https://learn.javascript.ru/coding-style. Постарайтесь использовать автоматическое форматирование, это сильно экономит время. Одно из наиболее популярных дополнений для форматирования кода - Prettier (https://prettier.io/)

  • Также заметил несколько слов написанных с ошибками. Чтобы это происходило реже попробуйте попользоваться расширением spell checker. https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker

Подробно эти и более мелкие замечания, отмечены в коде.

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

src/components/app/app.js Outdated Show resolved Hide resolved
return prevSum + i.price;
}, 0) : 0;
export function BurgerConstructor() {
const total = (bun, items) => {

Choose a reason for hiding this comment

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

Можно лучше:

Лучше обернуть в useMemo. Чтобы не было перерасчёта после ререндеров.

Copy link
Owner Author

Choose a reason for hiding this comment

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

функция обернута в редьюсер, и вызывется в useEffect только при первом рендере. Разве это не гарантирует, что её расчёт будет проводиться только один раз?

function reducer(state, action) {
return total(action.bun, action.items);
}

const [stateTotal, dispatch] = useReducer(reducer, 0);
useEffect(() => {
dispatch({bun: bun, items: mainItems});
}, []);

Choose a reason for hiding this comment

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

Всё верно, что-то я перепутал)) думал что это просто переменная, ну не суть.
Тогда лучше вообще эту функцию вынести в утилиты, ибо при каждом ререндере пересоздается ссылка на функцию)) А редьюсер лучше вынести за пределы компонента, по той же причине.

src/components/burgerConstructor/burgerConstructor.js Outdated Show resolved Hide resolved
Comment on lines 35 to 38
const allbuns = items.data.data.filter((i) => {
return i.type == "bun"
});
const bun = allbuns[Math.floor(Math.random() * allbuns.length)];

Choose a reason for hiding this comment

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

Можно лучше:

Для того чтобы вытащить булку, рекомендую использовать метод find.

https://developer.mozilla.org/ru/docs/Web/JavaScript/Reference/Global_Objects/Array/find

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Ну так нам пока необязательно чтобы булки менялись каждый раз) ну и плюс всё равно в будущем это всё будет происходить через DnD_ . В любом случае, это замечание не обязательно к исправлению.

src/components/burgerConstructor/burgerConstructor.js Outdated Show resolved Hide resolved
src/components/burgerConstructor/burgerConstructor.js Outdated Show resolved Hide resolved
src/components/burgerConstructor/burgerConstructor.js Outdated Show resolved Hide resolved
Comment on lines 41 to 42
<h2 className='text text_type_main-medium mb-6'>Булки</h2>
<div className={`${styles.ingridientsTable} pl-4 pr-4`}>

Choose a reason for hiding this comment

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

Можно лучше

у каждой категории ингредиентов дублируется верстка, лучше вынести отдельный компонент ingredients-category и переиспользовать его

src/components/burgerIngredients/burgerIngredients.js Outdated Show resolved Hide resolved
src/utils/api.js Outdated Show resolved Hide resolved
Copy link

@ArslanMustafin ArslanMustafin left a comment

Choose a reason for hiding this comment

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

Code Review

Здравствуйте!

Вы молодец! Работа принята. Можете приступать к выполнению следующего задания. Удачи в дальнейшем обучении.

  • Все замечания с прошлого ревью были исправлены.

Но есть несколько замечаний, которые лучше исправить перед выполнением следующего задания:

Сделает код лучше:

  • при клике на таб скролить список ингредиентов к нужному разделу.

  • Стоит обращать внимание на предупреждения в терминале, в котором запущен проект. Это хорошие рекомендации, которых следовало бы придерживаться. В противном случае, лучше отключить линтер у строки (куска кода) eslint-disable. Так же добавить комментарий (TODO), объясняющий причину блокировки линтера в данном месте. Суть в том, что мы стараемся писать более чистый код, а если видим, что порой выскакивают предупреждения того же линтера, то оставляем комментарий, объясняющий причину. В коммерческой разработке, когда есть целая команда разработчиков, работающих над одним проектом, эта рекомендация послужит правилом хорошего тона. Неплохая статься по поводу локального отключения линтера: https://learn.coderslang.com/ru/0023-eslint-disable-for-specific-lines-files-and-folders

Подробно эти и более мелкие замечания, отмечены в коде.

order: "https://norma.nomoreparties.space/api/orders"
};

export const loadDataToState = async (callbackApi, paramsApi, callbackState) => {

Choose a reason for hiding this comment

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

Можно лучше:

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

return items.data.data.filter((i) => {
return i.type === "bun";
});
}, []);

Choose a reason for hiding this comment

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

Надо исправить:

Здесь и далее.
У вас не будет происходить фильтрация, если вы оставите массив зависимостей пустым. Вам лучше передать туда items.data.data (и ещё я бы уменьшил как-нибудь эту надпись) нужно смотреть в сторону присвоение контексту значения)

@Tahora Tahora merged commit a9431e6 into master Dec 10, 2022
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