Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: удаление компонента Link #489

Merged
merged 3 commits into from
Sep 20, 2023
Merged

fix: удаление компонента Link #489

merged 3 commits into from
Sep 20, 2023

Conversation

AleksandrDronov
Copy link
Collaborator

@AleksandrDronov AleksandrDronov commented Sep 13, 2023 •
edited
Loading

Описание

Удаление компонента Link. Вместо удаления компонента InfoLink, удалил компонент Link по ряду причин:

  1. Компонент Link используется только в одном месте по коду, а InfoLink во многих. И в этом месте я заменил на компонент Link из next.
  2. Компонент InfoLink более универсальный. Имеет много пропсов, может принимать иконку и т.д.

Ссылка на задачу

Избавиться от компонента InfoLink

AlMkin
AlMkin reviewed Sep 13, 2023
View reviewed changes
Copy link
Collaborator

@AlMkin AlMkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

И давай тогда переименуем InfoLink в Link
Будет компонент Link который под все наши нужды можно настроить, и посмотри еще на Button, чтобы он не повторялся с Link
И добавляй еще плз скриншотики с компонентами, как что поменялось, если менялось

src/components/author-page/overview/overview.tsx Outdated
@@ -171,7 +171,7 @@ export const AuthorOverview: React.FC<IAuthorOverview> = (props) => {
{achievements.map((item, idx) => (
<div className={cx('tag')} key={idx}>
<Link href={`library/?festival=${item.year}&program=${item.id}`}>
<a>
<a className={cx('tagLink')}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай тогда лучше заязаем тут InfoLink, доработай его чтобы стилям соответствовал или пропсами или классом

@AleksandrDronov
Copy link
Collaborator Author

AleksandrDronov commented Sep 13, 2023 •
edited
Loading

И давай тогда переименуем InfoLink в Link Будет компонент Link который под все наши нужды можно настроить, и посмотри еще на Button, чтобы он не повторялся с Link И добавляй еще плз скриншотики с компонентами, как что поменялось, если менялось

Если переименовать, то у нас будет два компонента с одним именем. Link который импортируем из nexta и наш. Выглядит не очень.
С Button схоже. Можно по идеи все InfoLink заменить на Button. Видимо так и планировалось предыдущим разрабочиком.
Не понял какие скриншоты? С каким изображением

@AlMkin
Copy link
Collaborator

AlMkin commented Sep 14, 2023 •
edited
Loading

И давай тогда переименуем InfoLink в Link Будет компонент Link который под все наши нужды можно настроить, и посмотри еще на Button, чтобы он не повторялся с Link И добавляй еще плз скриншотики с компонентами, как что поменялось, если менялось

Если переименовать, то у нас будет два компонента с одним именем. Link который импортируем из nexta и наш. Выглядит не очень. С Button схоже. Можно по идеи все InfoLink заменить на Button. Видимо так и планировалось предыдущим разрабочиком. Не понял какие скриншоты? С каким изображением

И давай тогда переименуем InfoLink в Link Будет компонент Link который под все наши нужды можно настроить, и посмотри еще на Button, чтобы он не повторялся с Link И добавляй еще плз скриншотики с компонентами, как что поменялось, если менялось

Если переименовать, то у нас будет два компонента с одним именем. Link который импортируем из nexta и наш. Выглядит не очень. С Button схоже. Можно по идеи все InfoLink заменить на Button. Видимо так и планировалось предыдущим разрабочиком. Не понял какие скриншоты? С каким изображением

Так, ну в идеале конечно должен быть Link наш который и использует внутри next/link и дальше по проекту юзается уже наш Link, но там я уже смотрю много где next/link юзается напрямую, так что давай оставим InfoLink
а по поводу button, тут получается надо или все заменить на button или выпелить из button Link и те button которые использовались как link мигрировать на infolink

а это скриншотики, на которых можно подсветить, что поменялось. Например infoLink, Link, Button визуально могут быть похожи и чтобы мне не лазить по коду/ стенду и смотреть как они отличаются, ты можешь писать пояснение и подсвечивать какие-то моменты которые ты поменял по дизайну. Это обычная практика в командах, чтобы ревьюерам было легче и ревью шло быстрее.

Screenshot 2023-09-14 at 10 10 24

@AleksandrDronov
Copy link
Collaborator Author

AleksandrDronov commented Sep 14, 2023 •
edited
Loading

Хорошо.

  1. InfoLink оставляем
  2. По поводу button. Есть задача связанная с этим button. Использовать новую реализацию кнопки
    Давай я ее возьму и в рамках ее уже поменяю все старые button на новые и выпелю из button Link и те button которые использовались как link мигрирую на infolink

@AleksandrDronov
Copy link
Collaborator Author

@AlMkin Тогда этот ПР влей в develope.

@AleksandrDronov
Copy link
Collaborator Author

Заменил Link на InfoLink.
Снимок экрана 2023-09-16 в 12 44 53

@AlMkin AlMkin merged commit 25db681 into develop Sep 20, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlMkin AlMkin AlMkin approved these changes

@KonashinAlexander KonashinAlexander Awaiting requested review from KonashinAlexander

@Aleksey-dev-crt Aleksey-dev-crt Awaiting requested review from Aleksey-dev-crt

@Pyctam-AC Pyctam-AC Awaiting requested review from Pyctam-AC

Assignees

@AlMkin AlMkin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AleksandrDronov @AlMkin