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.

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

40177321: message feed layout #146

Merged
merged 8 commits into from
Nov 16, 2024
Merged

40177321: message feed layout #146

merged 8 commits into from
Nov 16, 2024

Conversation

Enchu
Copy link
Contributor

@Enchu Enchu commented Oct 28, 2024

No description provided.

Copy link

vercel bot commented Oct 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
xieffect ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 16, 2024 0:38am

Copy link
Contributor

@unknownproperty unknownproperty left a comment

Choose a reason for hiding this comment

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

Проблема с тем, что тулбар пропадает при нажатии на сообщение никуда не делась. Также, мне кажется есть отхождения от макетов

@@ -123,13 +92,13 @@ export const Chat = () => {
rootMargin: '400px 0px 0px 0px',
});

const scrollableRootRef = React.useRef<React.ElementRef<'div'> | null>(null);
const lastScrollDistanceToBottomRef = React.useRef<number>();
const scrollableRootRef = useRef<React.ElementRef<'div'> | null>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Я всё же советую получать хуки из React, а не импортировать их отдельно, это повышает читабельность кода. Можно легко определить, где кастомные хуки, а где из React

Loading
</li>
<div className="flex w-full p-2" ref={infiniteRef}>
<div className="mr-2 h-12 w-12 rounded-full bg-[#e1e3e4]" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Мы используем цвета из нашей палитры

<div className="flex-1">
<div className="flex items-center">
<div className="mr-2 h-6 w-36 rounded-lg bg-[#e1e3e4]" />
<div className="h-5 w-24 rounded-lg bg-[#e1e3e4] text-[14px] font-normal leading-[20px]" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Также стараемся использовать для типографии наши токены из конфига tailwind – https://github.com/xi-effect/xi.kit/blob/main/packages/pkg.tailwind/design-system-preset.js#L128

) : (
mocksMessages.map(
(item: MessageItemT, index: number) =>
item != null && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Всегда стоит использовать строгое сравнение

}}
>
<div className="flex w-full p-2 transition-colors">
<img className="mr-2 h-12 w-12 rounded-full" src="" alt="User Avatar" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Лучше сразу использовать компонент Аватара, в репозитории можно найти примеры

>
<MenuDots />
</Button>
</DropdownMenuItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

<DropdownMenuItem>
<Trash className="fill-red-60 mr-2 h-4 w-4" />
<span className="text-red-60 text-xs">Удалить</span>
</DropdownMenuItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

Странно, что при наведении не появляется ховер эффект на пунктах меню

onClick={() => console.log('search')}
>
<Search size="s" />
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Кнопку поиска я не добавлял, так как поиск мы будем делать в последнюю очередь

ref={(el: HTMLDivElement | null) => {
menuRefs.current[item.id] = el;
}}
className={`pointer-events-none absolute right-1 top-4 ${hovered === item.id ? 'pointer-events-auto opacity-100' : 'opacity-0'}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Постарайся использовать https://tailwindcss.com/docs/hover-focus-and-other-states#styling-based-on-parent-state, такой подход уже лучше, чем стейт в каждом сообщении, но он всё равно далеко не лучший

@Enchu Enchu changed the title fix(40177321): fixing hover effect fixing hover effect Nov 4, 2024
@@ -153,6 +122,48 @@ export const Chat = () => {
}
}, []);

const formatDate = (dateString: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Хм, а большой ли смысл держать в теле компонента эту функцию?

return dateFormat;
};

const shouldShowDate = (index: number, messages: MessageItemT[]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Тоже, что и выше

type="button"
className="flex h-12 w-12 items-center rounded-lg bg-[#445AFF] p-4 text-white"
>
<Send size="m" color="white" className="!important fill-white" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Что-то странное, мы так не пишем код. Что не так с иконкой?

</DropdownMenuItem>
<DropdownMenuItem>
<File className="mr-2 h-5 w-5" />
<span className="text-[14px]">Файл</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Можно использовать токены для текста из нашего конфига tailwind

className={`m-0 h-6 w-6 rounded p-1 ${
lockedHovered === item.id ? 'bg-gray-10' : 'hover:bg-gray-10'
}`}
onClick={() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Для чистоты кода я всегда советую выносить из атрибутов те функции, где больше одного действия

export const ChatMessage: React.FC<ChatMessageProps> = ({ item, index, mocksMessages }) => {
const { id, name, time, message } = item;

const getContainerClassNames = (
Copy link
Contributor

Choose a reason for hiding this comment

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

А в чём смысл держать эту функцию в теле компонента? При каждом ререндере компонента она будет создаваться вновь и вновь, тоже касается функций ниже

time: string;
};

export const DateChat: React.FC<DateMessageProps> = ({ index, mocksMessages, time }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Лучше не использовать FC, он может ломать типы, лучше точечно их настраивать

};

export const DateChat: React.FC<DateMessageProps> = ({ index, mocksMessages, time }) => {
const shouldShowDate = (index: number, messages: MessageItemT[]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

month: 'long',
});

return dateFormat;
Copy link
Contributor

Choose a reason for hiding this comment

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

Можно, сделать чуть красивее и возвращать сразу то, что на 28 строке, не создавая лишнюю переменную

};

export const SkeletMessage: React.FC<LoadingSkeletonProps> = ({ refProp }) => {
console.log('skelet message');
Copy link
Contributor

Choose a reason for hiding this comment

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

Забытый console

refProp: React.Ref<HTMLDivElement>;
};

export const SkeletMessage: React.FC<LoadingSkeletonProps> = ({ refProp }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Тоже тут нет смысла в FC

Copy link
Contributor

Choose a reason for hiding this comment

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

@unknownproperty
Copy link
Contributor

Тут появились конфликты, их стоит решить, сохранив изменения из ветки staging. Ещё в одном месте стоит вынести функции из тела компонента

@Enchu Enchu changed the title fixing hover effect 40177321: message feed layout Nov 16, 2024
@unknownproperty unknownproperty merged commit 30b3fd7 into staging Nov 16, 2024
6 checks passed
@unknownproperty unknownproperty deleted the 40177321 branch November 16, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants