Над чем нужно работать, что улучшать?

Question

[Марина]

Всем привет,

учу frontend, прохожу курсы Hexlet,
сделала свое первое тестовое задание от Aviasales на React:
https://marinarodkin.github.io/aviasales-app/
https://github.com/marinarodkin/aviasales-app

Как можно оценить мой уровень? Над чем нужно работать, в какую сторону? Буду делать еще несколько тестовых заданий для гит хаб. Цель - пойти на работу front-end'ом,

сама могу сказать, что как минимум, нужно доделать responsive, единицы измерения в CSS перевести в относительные,
логику фильтров упростить (но нужна подсказка как)

спасибо заранее за ответы!

Comments

[Павел Антонов]

можно почитать про styled components и использовать их)

[riddlr]

Тель-Авив - это твоя инициатива или еврейчиков из Хекслета?

Answer 1

[Владимир Проскурин]

1. Закомментированный код на гитхабе - не хорошо. https://github.com/marinarodkin/aviasales-app/blob...
   
2. Минимум логики в render функции компонента. Все сложные конструкции переносите в методы, а лучше в отдельные компоненты (тогда сможете легче контролировать перерисовку компонентов через shouldComponentUpdate , чтобы они не перерисовывались, если данные не поменялись). Вы можете прямо как методы писать стрелочные функции:
   

   class Flight extends Component {
       getWeekDay = (date) => {
           //..
       }
       // ....
   }

   
   
3. Вы в половине случаев используете точку с запятой, а в половине нет. Используйте чаще.
   
4. Атрибут for нельзя использовать в jsx (как и class, как вы знаете). Вместо for пишите htmlFor
   
5. Смотрите консоль инструментов разработчика, там есть ошибки.
   
6. Освойте shouldComponentUpdate, он позволяет контролировать перерисовку компонента при изменении состояния или пропсов. У вас при изменении кол-во пересадок, перерисовывается весь список билетов, даже те, которые уже были в этом списке. Многие скажут, что еще рано такое учить, но я не согласен. Если не учиться контролировать перерисовку еще в начале обучения, то можно написать очень много тормознутого софта.
   
7. У вас данные ticket.json подгружаются хардкодно из github, это не хорошо, т.к. этот файлик с данными есть в папке public, и если потенциальный работодатель захочет поменять там что-то, он не увидит изменений (т.к. грузится с гитхаба).
   
8. У вас если в данных в параметре departure_date стоит 11.10.2018 (т.к. сегодня), то отобразится это как "11 окт 2018, вс", т.е. день недели неправильный. А он неправильный потому, что это не октябрь, а ноябрь. Ошибка в методе getDateFormat
   

   const newDate  = new Date (year, month, day, );
   const monthName = ["дек", "янв", "фев", "мар", "апр", "мая", "июня", "июля", "авг", "сент", "окт", "ноя", "дек"];
   const newMonth = monthName[newDate.getMonth()];

   
   конструктор Date вторым аргументом ожидает номер месяца, нумерация которого начинается с нуля. т.е. 0 - январь, 1 - февраль, 11 - декабрь. Судя по monthName вы подозвевали, что есть что-то неладное, но ошибись с реализацией. monthName должен иметь обычный вид, начинаться с января и заканчиваться декабрем, т.к. нулевой элемент массива как раз подходит по логике с нулевым месяцем. В getDateFormat, а также в getWeekDay, вычтите из month - 1
   const newDate = new Date(year, month - 1, day)
   
9. У вас в тех же getDateFormat и getWeekDay в конструкторе Date вы в конце аргументов пишите запятую, так не нужно делать. Это не вредно и не полезно, просто дурной тон. Там в любом случае будет undefined, хоть с запятой хоть без нее.
   
10. Картинки тоже грузятся с marinarodkin.github.io, измените.
    
11. 
    

    const getStopsNumber = (stop) =>{
          switch (stop) {
            case 3:
              return "3 пересадки"
            case 2:
              return "2 пересадки"
            case 1:
              return "1 пересадка"
            case 0:
              return "без пересадок"
            default:
              return // это не нужно делать, писать return. Если вы удалите эту (и строку выше), то результат будет такой же - undefined
          }
        }

    
    
12. Если бы в SideBar пропс stopsData был не объектом, а строкой или числом, то компонент SideBar можно было бы безболезненно превратить в PureComponent. Ну это так, к слову об оптимизации.
    
13. Я бы в stopsClick передавал не объект события e, из которого вы потом берете id элемента через e.target.id (что не есть гуд), а сделал бы стрелочную функцию (или bind), в которую бы передавал id. Вот так
    

    <input onClick={() => this.props.stopsClick("allStops")} />
    <input onClick={() => this.props.stopsClick("noStops")} />

    
    Если это читают опытные ReactJS разработчики, рассудите пожалуйста. Согласен, что на каждый компонент будет создана своя копия функции, но по крайней мере, не нужно взаимодействовать с DOM напрямую.
    
14. Это не красиво
    

    if( this.state.stops.allStops === false && this.state.stops.noStops === true && this.state.stops.oneStop === true && this.state.stops.twoStop === true && this.state.stops.threeStop === true  ){
             newStops = {...this.state.stops, allStops: true}
        }

    
    мне кажется, на дальнейшую логику это никак не играет роли, лишь создает глюк, когда выбираешь все чекбоксы кроме "все", и если кликнуть после этого на один из них, он не отожмется, а лишь включится чекбокс "все".
    
15. Попробуйте везде сократить повторяющиеся конструкции. Например начните с stopsClick Не говорю, что у вас сразу получится, это приходит с опытом. Но просто попытайтесь подумать, как это можно сократить.
    

Может я многое высосал из пальца, но это будет вам полезно. Учитесь, развивайтесь. Удачи вам в этом :-)

Comments

[Марина]

спасибо!!!
очень подробно и очень полезно!

Answer 2

[Ivan Bogachev]

Не любитель реакта, поэтому про него не буду говорить. А вот CSS покритикую:
- Стоит прикрутить какой-нибудь препроцессор, поиспользовать вложенность (структура лучше будет видна) и вынести в человеко-понятные константы все, что выносится - цвета, размеры и.т.д. Там достаточно повторений.
- Стоит поделить все на отдельные файлы-компоненты.
- Стоит получше подумать над общим разбиением CSS на компоненты. Есть конечно разные подходы, но отдельные кнопки, или группа из нескольких кнопок, или чекбоксы - это универсальные штуки на весь проект. Какой смысл их привязывать к какому-то сайдбару или калькулятору?
- Про адаптивность вы сами написали.
- Стили для :focus отсутствуют. Клавиатурой не получится пользоваться.
- Еще мне кажется, что у сайдбара отступ внизу должен быть (дизайн не видел, но имхо есть). И что cursor: pointer у кнопок должен быть.

З.Ы.: Еще есть мысль, что вариант "все" там не нужен. "Все" должны показываться при отсутствии фильтров. Но без анализа ЦА не буду утверждать, может там к к такому варианту люди привыкли.

Comments

[Марина]

спасибо, погрузилась в js и React, уже подзабыла как там с препроцессорами, надо все повторять,
а логику фильтров я сделала как у авторов тест-задания на сайте.

[Макс]

На счет css - попробуйте styled-components как дойдут руки

[Ivan Bogachev]

Maxim, пробовал, получил головную боль, не заценил. Идея хорошая, но на мой взгляд не везде ее имеет смысл применять. Если компонентов немного, а на проекте 1 или 2 фронтендера, которые вполне могут договориться, то все эти подходы со смешением CSS и JS только все усложняют на ровном месте. Мне кажется, что собирать стили отдельно в такой ситуации - не самая плохая идея, хотя и не модная. Полагаю, что это только мое личное мнение, так что не стал в ответе ничего по этому поводу советовать или утверждать.

Answer 3

[spaceatmoon]

Ну вот чисто взгляд со стороны.

1. Научитесь оформлять проекты через markdown. Сейчас такое описание проекта тяжело читать, я осилил 5 слов, потенциальный работодатель вообще не посмотрел бы.
2. Группируйте файлы проекта по типу и смыслу. Сейчас это каша и трудно понять мне как простому не фронтендеру где логика, а где фреймфорк.
3. Так как вы пишите все еще говнокод, то приучайтесь над сложными участками кода писать комментарии где выражена суть исполнения функции.
4. Не знаю как во фронтенде сейчас, но бекендеры не любят когда мешают логику и шаблоны.
5. Научитесь раставлять скобки, прочитайте как оформлять код в js

spoiler

componentDidMount() {
    this.getTicketData("ticket.json");
      } // И так везде

6. Это что?

spoiler

newArr = [...newArr, ...arr0, ...arr1, ...arr2, ...arr3];

7. Это плохая практика. Код должен быт абстрагирован от данных. Нужно заменить на объект и проверять есть ли в объекте нужные данные или вернуть дефолтное значение.

spoiler

const getStopsNumber = (stop) =>{
      switch (stop) {
        case 3:
          return "3 пересадки"
        case 2:
          return "2 пересадки"
        case 1:
          return "1 пересадка"
        case 0:
          return "без пересадок"
        default:
          return
      }
    }

Пока хватит.

Comments

[spaceatmoon]

Еще бы добавил 8 пунктом это почаще делать коммиты. Сделали какую осмысленную правку и сделали коммит.

[Владимир Проскурин]

4. Не знаю как во фронтенде сейчас, но бекендеры не любят когда мешают логику и шаблоны.

Ахахахаха, сколько моей боли в вашем предложении. В ReactJS так и пишут...

[Марина]

спасибо за подробный разбор,
все поняла,
а 6-ой пп - это я фильтрую исходные данные в зависимости от выбранного кол-ва пересадок, а потом результаты собираю в один массив. Криво?

[spaceatmoon]

Марина, лучше один массив использовать.

Вот есть у вас такой массив к примеру:

var stop = {
one: {
 title: '1 пересадка',
 active: true
}
};

Вы просто меняется значение active и активируете фильтр.

[Толстый Лорри]

Владимир Проскурин, но ведь реакт - это слой представления, бизнес-логику оттуда вполне реально выносить. Или вы про человеческие слабости?

Answer 4

[DarthJS]

Добавлю. Вы работаете с Реактом, но при этом просто повставляли темплейты, что не имеет смысла, вы там можете взять абсолютно любой фреймворк и вообще без фреймворка закинуть темплейты и понавешивать хендлеры. Темболейт нужно разбивать на компоненты, в которые вы прокидываете данные через props, а так у вас много повторяющегося html
Поставьте линтер, он будет вам помагать. С ним вы будете хоть и говнокодить, но красиво

Comments

[Марина]

у меня там 2 компонента, один боковую панельку с фильтрами рендерит, другой - рейс.
значит нужно внутри панели с фильтрами каждый "пункт" сделать компонентом? в рейсах - компоненты "дата", "аэропорта"? или еще "мельче" - лого, кнопка, заголовок?
спасибо!

[DarthJS]

Один из первых загугленных примеров:
https://codepen.io/jessenoseworthy/pen/KawYod
Кнопка компонент с какими-то пропсами.
На сколько дробить это уже на своё усмотрение, может некоторые вещи этого и не потребуют.
Например в вашем коде sidebar_changing-block явно напрашивается на вынос в отдельный компонент. Ибо это уже какой-то спагетти код получается.
Особенно при использовании React девтулз вы сможете видеть более четку картинку, что куда идёт и какие параметры получает.

Answer 5

[Макс]

Заметил что у вас много функций определяется в методах render. В этом нет необходимости, а скорее нужно избегать - либо выносить на уровень класса, если нужен this, либо выносить за его пределы.
Еще вы ходите за картинкой и json файлом по ссылке на гитхаб, а можно их подключить через import, они ведь локально доступны для реакт приложения. Если с json еще спорно, то картинку в шапке точно нужно import-ить

Comments

[Марина]

import - очень актуальная подсказка !
я вообще застряла с картинками и json файлом при публикации на Git Hub pages, по относительным ссылкам они упрямо не подгружались, в итоге я ничего лучше не придумала как прямые ссылки вставить,
спасибо!

Answer 6

[Javid Asgarov]

Это вообще по js коду, чем чисто к реакту.

switch(curr){
      case "eur":
        newCurrency = {...this.state.currencyData, rub: false, usd: false, eur: true };
        break
      case "usd":
        newCurrency = {...this.state.currencyData, rub: false, usd: true, eur: false };
        break
      default:
        newCurrency = {...this.state.currencyData, rub: true, usd: false, eur: false };
        break
      }

А если будет еще валюта, будете еще свойства добавлять? Лучше держать свойство currency и от него отталкиваться. Был бы typescript, еще б указали какие строки туда можно куда присваивать. Причем в state currency уже есть, зачем currencyData я не понял.

Тоже самое со всеми stop свойствами, их много, трудно понять, что к чему. В общем, посмотрите на это в первую очередь.

Comments

[Марина]

эти блоки мне больше всего не нравятся, но по-другому не додумалась. Особенно пересадки. в случае валюты или-или, а в случае пересадок может быть любой набор - "1 пересадка + 3пересадки" например...

[Макс]

Такие штуки лучше хранить так:
Исходный массив допустимых значений и, если множественный выбор, массив выбранных пользователем значений.
А в рендере уже вычислять что тру, а что фолс и отрисовывать.