Feat: add button and NewsBlock to add more news to NewsPage #153
Conversation
| import dataIcon from '../../images/icons/ic_data.svg'; | ||
| import selfIcon from '../../images/icons/ic_self.svg'; | ||
|
|
||
| const NewBigCardVertical = ({ |
There was a problem hiding this comment.
suggestion(blocking): Если я правильно поняла, это компонент дублирует NewBigCard с другими scss свойствами. Мы можем переиспользовать компонент NewBigCard, добавив в него нужные нам изменения?
There was a problem hiding this comment.
Кроме изменения размеров фото и его положения на странице меняется порядок и расположение на нем html-элементов с темой/текстом/датой новости. Но сейчас экран в актуальном макете в фигме, по которому я это делала, вообще пропал.
There was a problem hiding this comment.
Это возможно: ту меняются преимущественно размеры. Мне нужна будет подсказка, как прописать эти изменения и остаться в общей стилистике кода)
There was a problem hiding this comment.
там Лера упорядочила все макеты, 1.22 -новости
| const NewCardVertical = ({ | ||
| id, header, publicationDate, shelter, image, | ||
| }) => { | ||
| return ( | ||
| <Link className='new-card-vertical' to={`/news/${id}`}> | ||
| <img className='new-card-vertical__image' src={image} alt={header} /> |
There was a problem hiding this comment.
suggestion(blocking):
Аналогично здесь.
Если я правильно поняла, это компонент NewCard с некоторыми другими scss свойствами. Есть ли возможность переиспользовать существующий компонент?
There was a problem hiding this comment.
Не могу придумать, как: стили там почти не меняются, но по макету (не увидела сейчас в фигме того фрейма, по которому делала) меняется порядок и вложенность html-элементов: там была строка из фото и колонки из трех текстовых единиц, стала колонка из фото, строки из двух текстовых единиц и третьей текстовой единицы.
| item.data.map((card, index) => { | ||
| return ( | ||
| index !== 0 && | ||
| index < 4 && ( | ||
| <li className='news-block__item' key={card.id}> | ||
| <NewCard id={card.id} header={card.header} publicationDate={card.pub_date} shelter={card.shelter} image={card.profile_image} /> | ||
| </li> | ||
| ) | ||
| ); | ||
| })} |
There was a problem hiding this comment.
suggestion(non-blocking): Есть хорошая практика маппинг выносить в отдельный компонент (я думаю, также нам давно пора сделать универсальный мап-компонент))🙃.
Также вынос этого кода поможет улучшить читаемость кода в этом компоненте.
Ниже повторяется этот кусок кода с разными параметрами index, было бы хорошо не дублировать код.
| const [newsToFetch, setNewsToFetch] = useState(4); // число новостей для подгрузки | ||
| const [isLoading, setIsLoading] = useState(false); | ||
| const [hasMoreNews, setHasMoreNews] = useState(false); | ||
| const [newsOffset, setNewsOffset] = useState(0); |
There was a problem hiding this comment.
suggestion(non-bloking): Возможно, стоит вынести в переменную или объект с полями, так как я поняла, всего несколько значений ставится - 4 или 7 для [newsOffset, setNewsOffset]
Например, вот так
const initialOffset = { smallOffset: 4, bigOffset: 7 }; - в логике не совсем разобралась, поэтому нейминг скорее всего не корректный предлагаю
There was a problem hiding this comment.
Логика тут работает так: в каждом новом блоке разное число новостей, и число единиц для подгрузки берется из этой переменной. То есть загрузили 4 - изменили в конце функции эту переменную на 7, при следующей подгрузке обращаемся к этой переменной с вопросом "сколько новостей подгрузить", подгрузили 7, изменили значение на 4, и т.д., в этом смысл того, что она одна и переписывается.
| useEffect(() => { | ||
| if (moreButton) { | ||
| moreButton.blur(); | ||
| } | ||
| }, [resList]); | ||
|
|
There was a problem hiding this comment.
question: можешь пояснить, что этот код делает? не совсем понятно. Также я не совсем поняла, зачем подключаться к кнопке через ref
There was a problem hiding this comment.
При нажатии на кнопку она оказывается в фокусе и по умолчанию остается в фокусе после подгрузки блока с новостями: до клика где-нибудь на странице у нее сплошная заливка цветом, на которой не читается текст, и этот useEffect снимает с кнопки фокус, если новости еще есть и кнопка отображается на странице.
Через реф - как-то без задней мысли: на курсе по реакту учили, что так компоненты более управляемые/контролируемые.
| app.shelterNewsOffsetById = {}; | ||
| app.shelterNewsToFetchById = {}; | ||
| app.hasMoreShelterNewsById = {}; | ||
| app.shelterNewsById[shelterId] = [{ data: resNews, type: newsToFetch }]; | ||
| app.hasMoreShelterNewsById[shelterId] = resNest; | ||
| setShelterNewsToFetch(); | ||
| } else if (app.shelterNewsById) { | ||
| if (app.shelterNewsById[shelterId] || app.shelterNewsById[shelterId] === 0) { | ||
| app.shelterNewsById[shelterId].push({ data: resNews, type: newsToFetch }); | ||
| app.hasMoreShelterNewsById[shelterId] = resNest; | ||
| setShelterNewsToFetch(); | ||
| } else if (typeof(app.shelterNewsById[shelterId]) === 'undefined') { | ||
| app.shelterNewsById[shelterId] = [{ data: resNews, type: newsToFetch }]; | ||
| app.hasMoreShelterNewsById[shelterId] = resNest; | ||
| setShelterNewsToFetch(); | ||
| } | ||
| } | ||
| app.shelterNewsById[fetchShelterId] = resNews; | ||
| } else if (!app.portalNews) { | ||
| app.portalNews = [{ data: resNews, type: newsToFetch }]; | ||
| app.hasMorePortalNews = resNest; | ||
| setPortalNewsToFetch(); | ||
| } else { | ||
| app.portalNews = resNews; | ||
| } | ||
| app.portalNews.push({ data: resNews, type: newsToFetch }); | ||
| app.hasMorePortalNews = resNest; | ||
| setPortalNewsToFetch(); |
There was a problem hiding this comment.
suggestion(blocking):
Очень трудно читать, когда так много условий и вложенности, и кажется, что есть проблема в подходе - как минимум мы пытаемся изменять значения из контекста напрямую (а так быть не должно)
Расскажи, пожалуйста, что здесь происходит и почему именно такой подход выбрала?
There was a problem hiding this comment.
До меня логика в общем блоке News была про то, что информация о подгружаемых пользователем новостях идет в контекст и они отобразятся полностью, если юзер переходит в другой блок, а потом возвращается в новости. С новостями приютов чуть сложнее, потому что приютов несколько: все нужные для отображения новостей переменные (уже загруженные блоки, остаток новостей на сайте, следующее число для подгрузки) сейчас тоже идут в контекст по Id приюта. Я взяла ту же логику внесения в контекст, которая была там, расширив на приюты и новые переменные, которые требуются для корректного отображения на странице.
В этой штуке отдельно логика сохранения для общего блока новостей и новостей конкретных приютов, с разбивкой на случаи, когда нужная переменная там уже есть и когда нужно вначале создать ее (так я продолжила унаследованную логику и поборола ошибку, которая вылетала при попытке убрать это разделение).
| )} | ||
|
|
||
| {hasMoreNews && resList !== [] && ( | ||
| <Button className='button_theme_transparent' onClick = {() => {return fetchNews(arg);}} disabled={isLoading} innerRef={moreButtonRef}> |
There was a problem hiding this comment.
| <Button className='button_theme_transparent' onClick = {() => {return fetchNews(arg);}} disabled={isLoading} innerRef={moreButtonRef}> | |
| <Button className='button_theme_transparent' onClick = {handleClick} disabled={isLoading} innerRef={moreButtonRef}> |
Co-authored-by: Tatiana Ponomareva <[email protected]>
Co-authored-by: Tatiana Ponomareva <[email protected]>
Co-authored-by: Tatiana Ponomareva <[email protected]>
| width: 79%; | ||
| margin: 0; | ||
| padding: 0; | ||
| font-size: 18px; |
There was a problem hiding this comment.
suggestion: у нас есть стили стандартные для шрифтов, находится все в standart-font.scss
здесь получается стили для шрифта body, т.е. можно в className прописать этот класс, а здесь убрать стили
| @@ -1,6 +1,6 @@ | |||
| .new-card__image { | |||
| width: 248px; | |||
| height: 142px; | |||
| object-fit: contain; | |||
| object-fit: cover; | |||
There was a problem hiding this comment.
thought: этот вариант мы как-то обсуждали, и получалось что при таком варианте обрезаются фотки. хотели это обсудить в дизайнерами и Даниилом, но как-то забыли...
здесь вот сурикат обрезается
| @@ -0,0 +1,7 @@ | |||
| @import url('./styles/new-card-vertical.scss'); | |||
There was a problem hiding this comment.
suggestion: раз работаешь с этими файлами, можно сразу в scss перевести)
| width: 100%; | ||
| margin: 0 0 12px; | ||
| padding: 0; | ||
| font-size: 18px; |
There was a problem hiding this comment.
suggestion: здесь аналогично по шрифту, можно убрать стили и прописать класс шрифта body
An error in shelter api: shelter ID don't changes the result