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

Sprint1/step1 #1

Merged
merged 8 commits into from
Jan 12, 2023
Merged

Sprint1/step1 #1

merged 8 commits into from
Jan 12, 2023

Conversation

miptleha
Copy link
Owner

@miptleha miptleha commented Jan 7, 2023

No description provided.

Copy link

@vadmas vadmas left a comment

Choose a reason for hiding this comment

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

Хорошая работа.
Приложение начинает приобретать готовый вид! Видно, что вы старательно относитесь к выполнению заданий.
В положительную сторону хочется отметить:

  • В проекте есть все необходимые компоненты
  • Стили портированы как модули
  • Активно используется библиотека @ya.praktikum
  • Верстка проекта соответствует макету
  • Компоненты описаны в функционально стиле
  • Реализована навигация по типам ингредиентов - молодец!

Но есть несколько замечаний:

Замечания которые необходимо будет исправить:

  • Компонент App (и одноименные файлы) уместнее вынести в директорию components в папку с одноименным названием так,
    как это реализовано с другими компонентами
  • Стоит придерживаться правила: один компонент - один файл - одна папка (и у всех одноименное название).
    В данной работе в файлах app-header.jsx, burger-constructor.jsx, burger-ingredients.jsx - реализовано несколько компонентов
  • Необходимо установить пакет prop-types в зависимости package.json

Так же стоит учесть комментарии, оставленные в коде.

Места, в которых можно улучшить проект:

  • Вычисления, фильтрацию и маппинд данных, выполняемых на стороне компонента, целесообразно оборачивать в хук useMemo.
    Так повторные пересчеты значений не будут выполняться на каждый перерендер компонента,
    а только при изменении установленных зависимостей.
    Подробнее о данном хуке можно почитать здесь: https://ru.reactjs.org/docs/hooks-reference.html#usememo
  • Скролл к выбранному разделу ингредиентов (при навигации), лучше сделать плавным

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

Удачного рефакторинга кода!

src/components/app-header/app-header.jsx Outdated Show resolved Hide resolved
src/components/burger-constructor/burger-constructor.jsx Outdated Show resolved Hide resolved
src/components/burger-constructor/burger-constructor.jsx Outdated Show resolved Hide resolved
src/components/burger-constructor/burger-constructor.jsx Outdated Show resolved Hide resolved
src/components/burger-constructor/burger-constructor.jsx Outdated Show resolved Hide resolved
src/components/burger-ingredients/burger-ingredients.jsx Outdated Show resolved Hide resolved
src/components/burger-ingredients/burger-ingredients.jsx Outdated Show resolved Hide resolved
src/components/burger-ingredients/burger-ingredients.jsx Outdated Show resolved Hide resolved
src/components/burger-ingredients/burger-ingredients.jsx Outdated Show resolved Hide resolved
src/components/burger-ingredients/burger-ingredients.jsx Outdated Show resolved Hide resolved
@miptleha
Copy link
Owner Author

miptleha commented Jan 8, 2023

Надеюсь не забыл исправить все замечания (было очень много).
Табы вынес в отдельный компонент, т.к. они постоянно перерисовываются (при смене вкладки - меняется состояние). Для того чтобы родительский компонент не перерисовывать с огромным списком (даже с учетом того, что список теперь в memo)

@vadmas
Copy link

vadmas commented Jan 9, 2023

Отлично, по первой части работы все критичные комментарии исправлены. Видно, что вы хорошо поработали!
Можно приступать ко второй (финальной) части работы.
Финальную часть работы лучше реализовать в новом pull-реквесте "sprint-1/step-2"

@miptleha miptleha merged commit dcd4180 into main Jan 12, 2023
@miptleha miptleha mentioned this pull request Jan 12, 2023
@miptleha miptleha deleted the sprint1/step1 branch January 15, 2023 14:43
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