feat: add adaptive and pagination #112

ponomareva-frontend · 2023-05-25T17:28:34Z

No description provided.

Artyom774

@stillNiceTanya, привет!

Проделана хорошая и объёмная работа, спасибо большое!

Оставил несколько комментарий по коду

Artyom774 · 2023-05-25T18:48:04Z

src/components/PaperCard/paper-card/__image/paper-card__image.css

-  max-width: 376px;
-  height: 252px;
-  object-fit: contain;
+  max-width: 288px;


Почему максимальная ширина у изображения карточки стала 288, если на макете есть 376px?

Спасибо, взяла не правильную ширину из мобильной версии

Artyom774 · 2023-05-25T18:51:23Z

src/components/PaperCard/paper-card/__text/paper-card__text.css

  padding: 0;
  font-size: 24px;
  line-height: 29px;
  font-weight: 500;
-  font-family: 'Montserrat Alternates', 'Inter', Arial, sans-serif;
+  font-family: "Montserrat Alternates", "Inter", Arial, sans-serif;


Мы всегда используем одинарные кавычки, так что менять незачем

Для шрифтов и цветов используем CSS-переменные, которые хранятся в файле https://github.com/Lapkipomoshi/help_paw_frontend/blob/dev/src/utils/vars.css
Конкретно здесь будет font-family: var(--font-family-title);

Artyom774 · 2023-05-25T18:58:44Z

src/pages/PapersPage/PapersPage.jsx

  }, []);

  return (
    <MainContainer>
      <main className='main papers'>
        <div className='papers__head-block'>
          <h1 className='papers__title'>Полезные статьи</h1>
-          <Button className='margin-left_auto' to='/shelters' link>Хочу помогать</Button>
+          <Button className='margin-left_auto button--mobile-hidden' to='/shelters' link>


Все классы называем согласно БЭМ-методологии. Название класса button--mobile-hidden не подходит
По БЭМ можно создать класс papers__offer-button и прописать нужные стили туда

Всем новые и изменённые файлы стилей можешь делать с расширением .scss

src/pages/PapersPage/PapersPage.jsx

Artyom774 · 2023-05-25T19:03:23Z

src/pages/PapersPage/PapersPage.jsx

@@ -5,30 +5,47 @@ import PaperCard from '../../components/PaperCard/PaperCard';
 import Button from '../../ui/Button/Button';
 import papersApi from './api';

+const PAPERS_PER_PAGE = 1;


Почему 1? Должна быть зависимость от разрешения окна браузера

Для констант используем верблюжий шрифт CamelCase. Исключения - ошибки и регулярки

сделала зависимость от разрешения окна такое mobile: 2, tablet: 4, desktop: 8
если где-то пропустила информацию, что другие цифры для первой подгрузки новостей, скажи, пожалуйста :)

Artyom774 · 2023-05-25T19:11:08Z

src/pages/PapersPage/PapersPage.jsx

 const PapersPage = () => {
  const [papersList, setPapersList] = useState([]); // список отображаемых карточек со статьями
+  const [papersPage, setPapersPage] = useState(0); // текущая загруженная страница статей


Зачем отдельная переменная состояния для этого? Достаточно использовать papersList.length

Artyom774 · 2023-05-25T19:12:41Z

src/pages/PapersPage/PapersPage.jsx

    papersApi
-      .getPapers(9) // загрузка статей
+      .getPapers(PAPERS_PER_PAGE, papersPage * PAPERS_PER_PAGE) // загрузка статей


Здесь точно надо умножать второй аргумент на PAPERS_PER_PAGE?

Artyom774 · 2023-05-25T19:18:47Z

src/pages/PapersPage/PapersPage.jsx

-        <Button className='margin_column-center'>Больше статей</Button>
+        <Button
+          className='button--full-width'
+          disabled={!hasMorePapers}


Кнопку лучше не деактивировать, а вообще скрывать - для этого есть класс display_none

расположение .display_none не перебивает .button {display:inline-block}, проще сделать условный рендеринг кнопки, в зависимости от того, есть новости или нет

еще как вариант можно сделать так же, как с показом надписи "Не удалось загрузить статьи". Если все новости отображены, то ничего не рендерить.
{papersList.length условие ? ( <Route path="/movies"> <Button className='margin_column-center'>Больше статей</Button> </Route> ) : ( "" )}

Artyom774 · 2023-05-25T19:23:13Z

src/pages/PapersPage/styles/__title/papers__title.css

+}
+
+@media (min-width: 768px) {


Можно добавить класс элементу standart-font_type_h2, тогда можно вообще не прописывать для шрифтов медиа запросы

Это стиль из файла https://github.com/Lapkipomoshi/help_paw_frontend/blob/dev/src/pages/App/styles/standart-font.scss

Artyom774 · 2023-05-25T19:28:21Z

src/pages/PapersPage/styles/__grid-element/papers__grid-element.css

+    min-width: 400px;
+    min-height: 416px;
+    padding: 8px 8px 24px;
+    border: 2px solid var(--color-background-base);


Можно или не указывать границу, раз уж она не отображается, или задавать цвет rgba(0,0,0,0). А заданный цвет вводит в заблуждение как будто граница есть

Artyom774 · 2023-05-26T04:22:26Z

src/pages/PapersPage/styles/__grid/papers__grid.css

+    gap: 36px 24px;
+    margin: 0 auto 60px;
+    padding: 0;


Стиль padding уже задавала ранее с тем же значением

Artyom774 · 2023-05-27T08:02:44Z

src/pages/PapersPage/PapersPage.jsx

      .then((res) => {
-        setPapersList(res.results);
+        setPapersList((prev) => { return [...prev, ...res.results]; });


Можно записать короче, setPapersList([...papersList, ...res.results]);

Artyom774 · 2023-05-27T15:49:16Z

src/components/PaperCard/paper-card/__text/paper-card__text.css

@@ -6,7 +6,7 @@
  font-size: 24px;
  line-height: 29px;
  font-weight: 500;
-  font-family: "Montserrat Alternates", "Inter", Arial, sans-serif;
+  font-family:  var(--font-family-title);


Тут лишний пробел

Artyom774 · 2023-05-27T15:49:32Z

src/components/PaperCard/paper-card/__text/paper-card__text.css

@@ -6,7 +6,7 @@
  font-size: 24px;
  line-height: 29px;
  font-weight: 500;
-  font-family: "Montserrat Alternates", "Inter", Arial, sans-serif;
+  font-family:  var(--font-family-title);
  color: var(--color-text-base);
 }



Лишняя пустая строка

Artyom774 · 2023-05-27T15:51:01Z

src/components/PaperCard/paper-card/__image/paper-card__image.css

+  }
+}
+


Лишняя пустая строка

Artyom774 · 2023-05-27T15:58:09Z

src/pages/PapersPage/PapersPage.jsx

 import './PapersPage.scss';
 import MainContainer from '../../components/MainContainer/MainContainer';
 import PaperCard from '../../components/PaperCard/PaperCard';
 import Button from '../../ui/Button/Button';
 import papersApi from './api';

+const PapersAmountByBreakpoint = { mobile: 2, tablet: 4, desktop: 8 };


Эту константу лучше перенести в PapersPage, так как только в нём и используется

На фронте первая буква в СamelCase должная быть строчная. Заглавная - только у классов и функциональных компонентов. Это принятая общая договорённость вообще среди общества фронтов, чтобы видеть сразу с чем имеем дело

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

до 633px (1 карточка в ряд) изначально загружать 3 карточки как на макете (я бы 5 загружал, правда) и добавлять по 3 карточки;

на отрезке от 634 по 750px (2 карточки в ряд) изначально загружать 6 карточек и добавлять по 2 (или 4) карточки;

если от 751px (3 карточки в ряд) изначально загружать 9 карточек и добавлять по 3 карточки (по-моему лучше 6, но кто меня спрашивал)

Примечание: надо иметь ввиду, что точки переходов зависят от стилей, поэтому могут поменяться, так что, возможно, есть решение получше

Ну тогда я тебя не поняла, так как ты написал как раз таки не в camelCase, а в СamelCase 😅

На счет количества загружаемых карточек, зачем такая сложная логика, на что это влияет с точки зрения пользователя? Учитывая называнные брейкпоинты и количество для них, выглядит как костыль 🙂 если поменяется дизайн, ширина карточки или еще что - все поменяется и придется менять логику в коде и поддержка будет услажнена.

Лишь написал, как считается делать грамотно. Можешь не воссоздавать. Про момент в случае смены дизайна - сам указал в примечании

Классы и функциональные компоненты - с большой буквы
Переменные и методы - с маленькой буквы

Можно в css задавать количество столбцов для grid, если не нравятся, что брейкпоинты для grid не совпадают с брейкпоинтами для разрешений в макете

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

Artyom774 · 2023-05-27T16:44:31Z

src/pages/PapersPage/PapersPage.jsx

+    default:
+      return PapersAmountByBreakpoint.desktop;
+    }
+  }, [isMobile, isTablet, isDesktop]);


Любопытное решение, особенно учитывая, что никогда не встречал useMemo и useMediaQuery. Код работает, выглядит вполне читаемо
Однако он подключает дополнительную библиотеку и использует нескольких констант, которые код, видимо, постоянно отслеживает, что не очень оптимизировано

Альтернативное решение, если интересно, - создать функцию computePapersAmount:

const computePapersAmount = (isStart = false) => { const documentWidth = document.documentElement.clientWidth; if (documentWidth < 634) { return 3; } if (documentWidth > 750) { return isStart ? 9 : 3; } return isStart ? 6 : 4; };

И в запросе тогда указывать:
.getPapers(computePapersAmount(isStart), papersList.length)
Аргумент isStart нужен, так как количество изначальных карточек отличается от того, сколько надо добавлять при нажатии на кнопку

Думаю, тут надо посоветоваться с @bevuxyna

вообще почитала, что в нашем случае цена использования useMemo выше цены вычислений, так что не знаю, есть ли смысл использовать
https://habr.com/ru/companies/otus/articles/696610/

думаю, useMemo действительно лишний тут
насчет создания функции с привязкой к dom, не соглашусь, что это более оптимизированно

можно попробовать через window.innerWidth

чем это лучше, объясните, пожалуйста)

я не знаю, чем это лучше, но раз мы отказываемся от useMemo, я предложила вариант, который знаю)

с помощью useMemo я думала мемоизировать константу, я уберу его)) на использование useMediaQuery использование useMemo не влияет

@bevuxyna, разница в том, что window.innerWidth не учитывает скроллы

Artyom774 · 2023-05-27T17:07:36Z

src/pages/PapersPage/styles/__grid/papers__grid.css

+  }
+}
+


Лишняя пустая строчка

Artyom774 · 2023-05-27T17:08:35Z

src/pages/PapersPage/styles/__head-block/papers__head-block.css

+  }
+}
+


Лишняя пустая строчка

Artyom774 · 2023-05-27T17:11:09Z

src/pages/PapersPage/styles/__title/papers__title.css

-  font-size: 40px;
-  line-height: 49px;
+  font-size: 20px;
+  line-height: 24px;
  font-weight: 600;


Стили font-size, line-height и font-weight уже есть в классе standart-font_type_h2, так что дублировать их не надо

Лишнее 2 строчки в конце

Artyom774 · 2023-05-27T17:13:21Z

src/pages/PapersPage/styles/papers.css

-  width: 1440px;
-  padding: 0 132px 80px;
+  width: 100%;
+  padding: 0 16px 32px;
  background-color: var(--color-background-base);
  box-sizing: border-box;
  font-family: var(--font-family-title);


font-family: var(--font-family-title);
Эта строка уже тут лишняя. Удали, пожалуйста

Artyom774 · 2023-05-27T17:16:24Z

src/ui/Button/Button.scss

@@ -11,7 +11,7 @@
  border: none;
  border-radius: 12px;
  box-sizing: border-box;
-  transition-duration: .7s;
+  transition-duration: 0.7s;


Если каждый будет менять общие файлы на свой вкус, то у них будет забитая история коммитов, что может только мешать
В таких случаях надо или смотреть в доках, или искать договорённости в истории, или обсуждать, чтобы код поддерживать в одном стиле

Artyom774 · 2023-05-27T17:20:52Z

@stillNiceTanya, здорово, спасибо большое!

Возникло несколько предложений что можно улучшить

Artyom774

Так и не понял, зачем создавать 2 постоянных слушателей useMediaQuery вместо того, чтобы смотреть какая ширина у document 1 раз при загрузке страницы и после только при нажатии на кнопку

PapersContent выглядит неплохо, если сделать его универсальным и использовать на других страницах, а так не особо понимаю зачем выносить в отдельный компонент

На созвоне просили всегда добавлять по 3 карточки со статьями и изначальное количество сделать как на макетах

У isError всегда значение false и никогда не меняется, так что смысла в нём не вижу

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

ponomareva-frontend · 2023-05-29T16:42:56Z

Так и не понял, зачем создавать 2 постоянных слушателей useMediaQuery вместо того, чтобы смотреть какая ширина у document 1 раз при загрузке страницы и после только при нажатии на кнопку

PapersContent выглядит неплохо, если сделать его универсальным и использовать на других страницах, а так не особо понимаю зачем выносить в отдельный компонент

На созвоне просили всегда добавлять по 3 карточки со статьями и изначальное количество сделать как на макетах

У isError всегда значение false и никогда не меняется, так что смысла в нём не вижу

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

ponomareva-frontend · 2023-05-29T17:24:24Z

@Artyom774
исправила логику добавления количества статей. useMediaQuery - удобный хук, не совсем понимаю, чем следить за обновлением ширины экрана пользователя напрямую удобнее, чем использовать хук🙃

все-таки вынесла обратно вне компонента PapersPage константы initialPapersAmountByBreakpoint и papersAmount , так как у этих констант нет зависимостей от PapersPage, и не нужно, чтобы каждый раз был их перерендер

У isError добавила логику true, спасибо, что заметил, я пропустила😌

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

src/ui/CheckboxesSelect/CheckboxesSelect.jsx

src/components/PapersGrid/PapersGrid.jsx

ponomareva-frontend changed the base branch from main to dev May 25, 2023 17:29

ponomareva-frontend requested a review from Artyom774 May 25, 2023 17:40

Artyom774 reviewed May 25, 2023

View reviewed changes

Artyom774 reviewed May 26, 2023

View reviewed changes

Artyom774 reviewed May 27, 2023

View reviewed changes

ponomareva-frontend requested a review from Artyom774 May 27, 2023 14:10

Artyom774 reviewed May 27, 2023

View reviewed changes

bevuxyna requested review from bevuxyna and Artyom774 May 28, 2023 16:14

bevuxyna approved these changes May 28, 2023

View reviewed changes

TatianaP added 5 commits May 29, 2023 13:44

style: add adaptive to PaperPage

1b683cc

feat: add pagination for article request

2835bf3

'fix: PR comments'

d80cfd9

fix: eslint formatting error with SwitchCase

f7358c1

fix: PR comments part 2

7f84f5b

ponomareva-frontend force-pushed the papers-page-adaptive branch from 77cddef to 7f84f5b Compare May 29, 2023 15:10

ponomareva-frontend requested review from Artyom774 and removed request for Artyom774 May 29, 2023 15:17

Artyom774 approved these changes May 29, 2023

View reviewed changes

Artyom774 requested a review from bevuxyna May 29, 2023 16:04

ponomareva-frontend closed this May 29, 2023

ponomareva-frontend reopened this May 29, 2023

fix: PR comments part 3

a2ff190

bevuxyna approved these changes May 30, 2023

View reviewed changes

src/ui/CheckboxesSelect/CheckboxesSelect.jsx Show resolved Hide resolved

src/components/PapersGrid/PapersGrid.jsx Show resolved Hide resolved

refactor: PR comments part 4

5a37739

ponomareva-frontend merged commit 6fad85e into dev May 30, 2023

ponomareva-frontend deleted the papers-page-adaptive branch June 5, 2023 07:09

feat: add adaptive and pagination #112

feat: add adaptive and pagination #112

Conversation

ponomareva-frontend commented May 25, 2023

Artyom774 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artyom774 May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Artyom774 May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artyom774 May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artyom774 May 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artyom774 May 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artyom774 commented May 27, 2023 • edited Loading

Artyom774 left a comment • edited Loading

Choose a reason for hiding this comment

ponomareva-frontend commented May 29, 2023

ponomareva-frontend commented May 29, 2023

Artyom774 left a comment •

edited

Loading

Artyom774 May 25, 2023 •

edited

Loading

Artyom774 May 25, 2023 •

edited

Loading

Artyom774 May 26, 2023 •

edited

Loading

Artyom774 May 27, 2023 •

edited

Loading

Artyom774 May 27, 2023 •

edited

Loading

Artyom774 commented May 27, 2023 •

edited

Loading

Artyom774 left a comment •

edited

Loading