Привет, участникам и гостям. Ниже текстовая версия разбора третьего тестового задания.
Так же есть видео версия разбора:
Если ошибка / хороший ход повторяются — в общем ревью это не указывается во второй, третий и так далее разы. Для некоторых моментов, я все же повторяюсь, это сделано либо по невнимательности, либо мне хочется заострить внимание на отличном решении / проблеме.
try/catch / ErrorBoundaries. Чаще всего, это было связано с ошибкой загрузки библиотеки для google sign in.Как проверить автора новости?
Для сравнения авторства, если автор новости заходил через google:
/api/v1/auth/googleСобственные замечания в readme, не разбирал google jwt
У новости троеточие, даже если она короткая.
В просмотре новости, текст так же обрезается.
Сессия не сохраняется между рефрешами.
Подписка на весь store
Использование длинных названий переменных через ., более одного раза
Можно не биндить в конструкторе, при использовании стрелочных функциий
NewsItem грязноват, например: один раз проверено this.props.detail и потом еще доп.проверки, возможно недорефакторено
Опять лишняя проверка, если в cdm сделана проверка на новость, то нет смысла в шаблоне еще раз проверять это
let вместо const
Для методов delete & edit можно было бы заранее настроить axios заголовки, или вынести работу с API в отдельный модуль. (будет показано в следующей работе у Виталия Володенко)
После удаления новости — редьюсер пытается сделать/знать больше чем нужно, так как происходит редирект на главную страницу, где новости снова запрашиваются.
Есть proptypes.
Передача флага editable в тупой компонент
Использование сокращенной записи React.Fragment
Есть обработка ошибок, но нет уведомлений (о чем в readme признались)
Есть хелпер для даты
Не сторонник «веселых» исполнений, но в данной работе стиль выдержан полностью, а не частично, поэтому отнесусь к этому нейтрально с юморком.
Образцовая работа
Не найдено
Есть демо
Есть env.example файл
Бэк подпилен под свои нужды — можно было бы указать, под какиие.
Использование ErrorBoundary
Если новости отсутствуют
API для запросов + пример, когда запрос под api не подходит
Прокачанный хелпер для дат
Можно было бы вынести только state.user и не углубляться ниже.
Так же, работа сильная, можно добавить использование селекторов.
Из письма:
С реакт работаю 6 месяцев, с JS — 10 месяцев, с TS — 2 месяца.
Выполнил бы еще и бонусное задание, но уже устроился на работу, поэтому будет некогда.
.env файл не в .gitignore
Поехала верстка
Иконки не очень аккуратно сверстаны.
При удалении новости не спрашивается подтверждение. Всегда для удаления чего-либо — должно быть подтверждение, если заказчик не требует иного специально.
В reducers не используются константы, хотя они описаны.
Вместо того, чтобы добавлять новый флаг для условия в render, можно было взять курс на reducer новостей, и проверять флаг оттуда, но так как в reducer не хватает isFetching флага и ошибки — то решение со state допускается, хотя я бы лучше «прокачал» reducer.
NewsContainer подписан на user, но user не используется. Так же это потенциальная проблема для производительности, так как при изменении в user будет перериисовка и в NewsContainer.
Рекомендую использовать более подробную запись для массива с объектами. Подробнее на StackOverflow + документация
import PropTypes from 'prop-types';
ReactComponent.propTypes = {
arrayWithShape: PropTypes.arrayOf(PropTypes.shape({
color: PropTypes.string.isRequired,
fontSize: PropTypes.number.isRequired,
})).isRequired,
}
Нельзя сравнивать авторство по не уникальному атрибуту (в данном случае, по имени).
Ошибки показываются клиенту
Возможно, когда работа была послана, этого предупреждения еще не было, но тем не менее, не забывайте проверять свои зависимости, с помощью npm audit
Нет иконок в карточке новости, на странице всех новостей.
Избыточное условие в проверке, компонент не рисуется без news.
Создание новости с графическим редактором
Аккуратно в API описан signIn + использованы cookies. Наконец-то, сессия не сбрасывается!
Helper getDecodedUser — как раз, показано как можно вытащить id (отсылка к предыдущей работе), по которому потом сверять(?).
Обыденные hoc‘и, но хочу отметить их.
Передача actions в короткой записи
Кнопка «Назад».
Readme ок! Забыл лишь про env файл.
Методология Atomic
В письме было указано, что времени на полировку не хватило, но ревью от этого «мягче» не стало, так как работа сильная.
У формы входа отсутствует валидация. Например, не «отключаются» (disabled атрибут) кнопки, если поля пустые. Хотя у добавления новости — валидация есть.
Нет возможности вернуться на главную страницу с формы входа
Если пользователь что-то ввел в новости, и жмет «отмена» — не спрашивается подтверждение.
Оформление работы!
Используется годная библиотека для уведомлений react-tostify
Подтверждение удаления новости присутствует.
Используется отдельный layout для некоторых роутов
Интересный HOC withFeedRemove
Использование title для иконок (внимание к деталям!)
Использование extraArgument в redux-thunk, затем переменную api можно вызывать так:
(внимание на третий аргумент, документация)
Решение проблемы с инициализацией google api (библиотека будет точно загружена к моменту выполнения логина).
Сокращение T для PropTypes
Образцовая работа
Есть демо и скриншот в readme.
Новости появляются с анимацией, есть анимация на иконках.
Чтобы запустить проект, нужно не только создать .env файл с ключом, но еще и заменить в config API_URL
Кнопка добавить новость — не очевидная.
Редьюсер новостей и новости совмещен. Это две разные сущности, поэтому лучше их разбить на свои отдельные редьюсеры.
Решение с middleware по проверке токена + проверка токена в момент загрузки приложениия
Ошибки переведены для пользователей
Наличие ui-компонентов. Но я бы добил эту затею, например, post_button состоит из компонента кнопка/ссылка, определенного цвета и текста. Либо вместо одного компонента «кнопка+ссылка», сделать два «кнопка» и «ссылка».
Хак(?) для редиректа после удаления новости
Флажок статуса загрузки (loading) сделан непривычным для меня способом.
В решении используются immutable и саги. Подход — ducks.
Добавлены некоторые «плюшки», например — поиск новости по заголовку.
Мертвый код в комментариях
Скорее минус, чем нейтрально: проверка истечения токена (так как приходится дублировать. На крайний случай, можно было бы вынести в хелпер повторяющуюся часть). Сократить количество кода в компонентах и не только, поможет иное решение, например проверка в middleware, как это сделано у Михаила.
Много «буков», если так можно выразиться. Например, для получения новости. Проблема уже известная — снова все новости и конкретная новость объединены в один редьюсер. Нужно разделить. Затем простой запрос за новостью в cdm и подписка на редьюсер новости в mapStateToProps.
Повторяющийся код (валидация, вытаскивание переменных и тд тп) — лучше выносить в хелперы. Например, повторение в строках 91-93 и 173-175 в ducks/auth.js
Так же про повторение и для селекторов. Можно было бы добавить это в отдельный файлик и не дублировать код. Но за использование селектора — плюсик.
Общий шаблон для создания/редактирования новости. Стоит отметить, что с ростом разницы между этими формами, может расти сложность шаблона и в какой-то момент, может быть выгоднее сделать 2 разных компонента.
Используется Record вместо привычного мне Map, может быть автор прояснит плюсы?
Итого: считаю, что после данного обзора у автора будут мысли для рефакторинга и работа станет меньше по объему символов и легче для чтения.
Используется redux-api-middleware
Лишний контейнер
Нет обработки request / errors
Адрес API не вынесен в константу
Опасные имена для сохранения в LS, которые с легкостью может перезаписать другое приложение. Рекомендую использовать уникальный префикс.
Данные нормализуются
в итоге удаление превращается в легкий delete по ключу
Json.stringify для более безопасного сохранения новости
Решение проблемы с «еще не загруженным» api для google sign in
Warning при сборке
Warning: postcss-cssnext found a duplicate plugin (‘autoprefixer’) in your postcss plugins. This might be inefficient. You should remove ‘autoprefixer’ from your postcss plugin list since it’s already included by postcss-cssnext.
Кнопка «Войти» меняется на «Выйти», даже если ошибка получения токена (причем, уведомление с ошибкой показывается ОК)
В подробностях открывается не та новость. Вероятно из-за опечатки. С нормальным сравнением, работает.
cdm расположен ниже метода render. Это не ошибка, но большинство проектов пишутся опираясь на eslint-правило в котором указан порядок следования методов.
Много кода для перехода на редактирование новости, начинается через state. Нужно использовать Link из react-router
Плохое условие для показа кнопок редактирования/удаления
Токен вытащен напрямую, по документации нужно использовать:
googleUser.getAuthResponse().id_token
Компонент кнопки вынесен отдельно
В работе используются саги и Ant design
Проверка доступности редактирования/удаления новости сделана в самой новости. Компонент новости должен просто принимать флаг, например: editable — true/false. В данный момент, он знает больше чем положено.
[придираюсь] Иконка глаза не очевидна для редактирования
JSDoc дамы и господа!!!
Понятный текст ошибок
Работу можно рассматривать как хороший пример для работы с сагами.
Шаблон новости (короткая, полная версия) в одном файле.
Проект сделан с 💜
Если параметров у функции много, лучше класть их внутрь объекта, чтобы не «запоминать» порядок.
Нет автоформатирования кода, или автор использует непривычные для меня настройки, например строки 25-40 в NewsContainer.
Неуместные операции в редьюсере. Редьюсер, только получает и выдает данные. Все остальное делаем в Action Creator (создателе действия). Преобразование данных вполне можно делать в редьюсере.
Bind в констукторе — устарело. Можно просто использовать стрелочные функции для методов.
В работе выполняется поиск по заголовку, но для поиска всегда необходимо использовать trim и toLowerCase методы.
Избыточная запись, можно указать LoginPage в вызове connect.
cwm «устарел» (deprecated). Куда вынести эти расчеты рекомендации нет, нужно обсудить «желание» автора и подумать.
переменная expires — const (eslint спасет от таких проблем) (данный файл, вообще, не используется в проекте)
Выполнен дополнительный функционал, дизайн приятный, разбита на множество компонентов… (и в целом работа неплохая), но выше уже были описаны схожие плюсы, поэтому не повторяюсь.
NewsContainer с вложенными роутами, включая рендер роута через функцию
Вижу намек на cache — это здорово, но я не заметил как кэш сбрасывается(?)
Не все зависимости указаны в package.json
Не подчищен console.log
Вновь отмечу, что компонент пытается выяснить, можно ли показывать кнопку «редактирования» + ошибка, что выяснение идет не по уникальному свойству.
React.Fragment и <> на проекте. Хотелось бы в едином стиле.
Вероятно «старый код«, либо неясно назначение isEditing, который всегда true
Опечатка в названии функции (должно быть feedsReducer)
Поскольку ниже в плюсах отмечен Flow, хотелось бы большего покрытия. Как минимум, это обязательно (так как это просто) покрытие propTypes, ниже указана библиотека для этого.
Flow! Для компонентов, рекомендую использовать babel-plugin-flow-react-proptypes
Использование recompose отмечу как плюс. Хотя скорее всего, это привычный для автора способ. С одной стороны выглядит красиво и компоненты очень «тупые», с другой стороны — приходится бегать по файлам, чтобы подсмотреть примитивные обработчики.
Константы с namespace‘ом (область именования)
Работу предлагаю к рассмотрению как туториал по работе с recompose
В Readme есть подробное описание. Частично цитирую:
Проект создан с помощью структуры react-modern — это моя реализация фрактальной структуры приложения. Это мой первый опыт написания приложений на основе этой структуры, так что удалось найти некоторые проблемы и баги в ней. Само собой следующие приложения, написанные с этой структурой, будут более доработанными и красивыми.
О тестах вспомнил только на середине пути, так что не стал их писать.
Евгений, этот мотивирующий минус для тебя лично.
Можно ли обойтись без таймера? Считаю их ненадежными.
подстановка токена (но плохое имя у переменной в LS)
Работа с уведомлениями
Использование Styled-components
Зачем во всех названиях call? Чтобы отличить, что это http-вызов?
Еще раз отмечу, подход react-modern. Кому интересно — welcome в работу Евгения.
Посмотрите остальные работы и проанализируйте плюсы/минусы, указанные в review.
Спасибо за участие и до встречи в 2019м!
Сегодня будем использовать parcel и IntelliJ IDEA Community Edition. Все инструменты бесплатные. Инициализация elm проекта…
На данном вебинаре мы знакомились с языком Elm проводя параллели между Elm и Redux, поэтому…
Richard Feldman рассказывает как масштабировать Elm приложение без боли. Показаны техники: extended records, подход narrow…
В данной заметке вы найдете конспект видео по Elm, которые я посмотрел в ноябре 2019.…
Итоги года 2019 // Max Frontend Покажи мне свой гитхаб, и я скажу работал ли…
Почему стоит изучать Elm? Потому что это интересный вызов, редкие (но вкусные) вакансии и хороший…
View Comments
крутой материал! непонятно почему нет комментов )
:cry:
Ленивые все
Спасибо, отличный материал! Узнал много нового :smile:
Надеюсь будет 4-е задание, хотелось бы поучаствовать :smile: