Refactoring#1
Conversation
gentslava
left a comment
gentslava
left a comment
There was a problem hiding this comment.
Комментарии по ревью
|
|
||
| const showError = (internalErrors, errorsServer) => { | ||
| useEffect(() => { | ||
| if (!errorsServer && !errorsServer.isArray()) return null; |
There was a problem hiding this comment.
Потенциальная ошибка. Базово в js у объектов нет метода isArray().
Здесь несколько вариантов:
- Это ошибка и предполагается Array.isArray(errorsServer).
- Этот метод определен в объекте errorsServer.
- Этот метод определен через прототипирование.
Нужно понимать, какой у нас случай.
По хорошему, вопросов бы не возникло, если бы errorsServer был подробно типизирован, а не просто errorsServer: PropTypes.array.
Для этих целей нужно подробнее через propTypes расписать поля объекта, либо использовать ts.
| } = props; | ||
|
|
||
| const showError = (internalErrors, errorsServer) => { | ||
| useEffect(() => { |
There was a problem hiding this comment.
Перенес в useEffect, чтобы не было лишних срабатываний на каждый рендер.
| sendAgentCard, | ||
| // surnameSuggestions, | ||
| // firstNameSuggestions, | ||
| // patronymicSuggestions, |
There was a problem hiding this comment.
Комментарии обязательно удаляем. Все есть в git.
| </FormRow> | ||
| </> | ||
| )} | ||
| {false && props.username === userInfo.username && ( |
There was a problem hiding this comment.
Пусть false не смущает. Его убирать не нужно, раз вопросов о нерабочей функциональности не возникала.
Это банально отключённая функциональность = закомментированный код. Не долго думая, удаляем.
| } | ||
| }); | ||
| }; | ||
| }, [internalErrors, errorsServer]); |
There was a problem hiding this comment.
Вообще в этой функции происходят мутации объектов, что является анти паттерном React. Дочерний компонент мутирует объекты, которые приходят из родительского компонента. Такое сложно отлаживать, если дочерний компонент занесет ошибку.
Учитывая, что здесь просто происходят какие-то пробросы из одних ключей в другие, имеет смысл эти ключи добавить в валидатор, а данную функцию просто выпилить.
| whatsappBotEnable: "Подключить бота Whatsapp", | ||
| whatsappNotifyDisable: "Отключить уведомления Whatsapp", | ||
| whatsappNotifyEnable: "Подключить уведомления Whatsapp", | ||
| juridicalError: "Если вы хотите работать как юридическое лицо, то необходимо заключить с нами договор. Для этого вам нужно обратиться в тех. поддержку по телефону +7 (800) 550-50-43, либо написать на почту docs@rosstrah.ru, в теме письма указав свой логин", |
There was a problem hiding this comment.
Отображаемый текст выносим по той же причине. Плюс есть потенциал для локализации на разные языки.
| verificationPhoneModal: "verificationPhoneModal", | ||
| success: "success", | ||
| error: "error", | ||
| text: "text", |
There was a problem hiding this comment.
Константные строки лучше вынести в константы, чтобы сложнее было опечататься в названиях и было проще отладить такого типа ошибки.
| value={userInfo.email} | ||
| onChange={(value) => setValue([MAGIC_STRINGS.userInfo, MAGIC_STRINGS.email], value)} | ||
| onBlur={(value) => { | ||
| validate(MAGIC_STRINGS.email, value); |
There was a problem hiding this comment.
Функции в onChange, onBlur можно вынести в отдельные хендлеры. Тогда компонент будет чище, логика и отображение будут разделены.
| @@ -0,0 +1,146 @@ | |||
| import React, { memo } from "react"; | |||
| import moment from "moment"; | |||
There was a problem hiding this comment.
Библиотека moment.js deprecated. Можно использовать более легковесную библиотеку day.js
| if (loading) return ( | ||
| <EmptyNoBorder> | ||
| <Spinner /> | ||
| </EmptyNoBorder> |
There was a problem hiding this comment.
<EmptyNoBorder><Spinner /></EmptyNoBorder>
можно вынести в отдельный компонент <Loader />, который будет в UI-kit проекта.
1 participant
Рефакторинг компонента.
История изменений https://github.com/rosstrah/frontend-test/pull/1/commits