-
Notifications
You must be signed in to change notification settings - Fork 9
MV-294 Feature/sidebar component (desktop) #136
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some UI problems that need to be addressed. Details below
<div | ||
className={clsx( | ||
isSidebarOpen ? "max-w-[300px]" : "max-w-0", | ||
"overflow-hidden transition-all duration-300", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dramatic_zoom.mov
The transition looks fine by itself but it makes a dramatic zoom in on almost every video grid layout. I think it is a little distracting but it should be discussed with others whether we want to leave this animation, make a workaround for this effect or leave it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not related to animation but to the change of width that is occupied by the grid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it is strange to look at. Google meet has a very nice animation for that and a very thoughtful design for this. I think we should consider developing something similar in future.
assets/src/pages/room/RoomPage.tsx
Outdated
<section className="flex h-full w-full flex-col"> | ||
<section | ||
className={clsx( | ||
"flex h-full w-full self-center justify-self-center 3xl:max-w-[1728px]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the change that has just landed on master (#140)
{ | ||
id: "chat", | ||
icon: Chat, | ||
hover: isSidebarOpen ? "Close the chat" : "Open the chat", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think of what to say here, but since we don't have chat this isn't a helpful tooltip. I'd stick to "Close/Open the sidebar" for now
It adds a sidebar component (without chat) on the desktop dimensions. The view when the sidebar is opened and users are pinned is incompatible with Figma design for now. This use case might be handled as a separate task
vs
What's more, on larger screens, the x-axis padding doesn't look good when the sidebar is open. The design for this usecase requires some changes.