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

Month 13/step 1 #14

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Conversation

ulbee
Copy link

@ulbee ulbee commented May 31, 2023

No description provided.

ulbee added 28 commits May 6, 2023 15:22
Copy link

@andryxxa93 andryxxa93 left a comment

Choose a reason for hiding this comment

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

Здравствуйте! Проделана большая работа 👍

Что понравилось:

  • Все jest и snapshot тесты проходят успешно
  • Избежали дубликатов кода в jest тестах

Однако есть пару моментов, которые необходимо исправить, чтобы работа была принята.

Необходимо исправить:

  • Проект собирается с ошибкой из-за этого cypress тесты не могут пройти. Возможно вы забыли добавить package.json в коммит.
Снимок экрана 2023-05-31 в 18 56 20
  • http://localhost:3000/ повторяется несколько раз в коде тестов. Нужно вынести в константу testUrl, чтобы можно было одним движением изменить урл для тестов
    А можно в документации посмотреть, как настроить быстро baseUrl для cypress https://docs.cypress.io/api/commands/visit#Visit-is-automatically-prefixed-with-baseUrl

  • form input[type="text"] - повторяется 11 раз

  • form button[type="submit"] - повторяется 8 раз.

  • Если строка-селектор повторяется более 2х раз в коде, значит, этот селектор нужно выносить в константу, чтобы можно было одним движением изменить его и не искать по всему коду дублирования этого селектора.

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

Удачных правок!

@ulbee
Copy link
Author

ulbee commented May 31, 2023

Спасибо большое за ревью!
Постаралась исправить все аккуратно. Также поставила в зависимости плагин, из-за которого не собирается приложение, только я попробовала все с нуля собрать у себя и необходимости в этом плагине не было и все тесты и приложение запустились...
Я не очень поняла, как такое возможно. Надеюсь, сейчас все соберется.

@andryxxa93
Copy link

Супер, вы отлично поработали!
Поздравляю! Работа принята.
Желаю удачного продолжения обучения!

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