Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix: No way to open chat in video rooms with new header. #11752

Closed
wants to merge 1 commit into from
Closed

Fix: No way to open chat in video rooms with new header. #11752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions res/css/views/rooms/_RoomHeader.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,9 @@ limitations under the License.
background: var(--cpd-color-bg-subtle-primary);
}
}
.mx_RoomHeader_ChatIcon {
* {
fill: $icon-button-color;
}
Comment on lines +91 to +93
Copy link
Member

Choose a reason for hiding this comment

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

it seems like it would be better to update the SVG to have fill="currentColor" than to forcibly override it here. Generally, I don't think it should be necessary to have a dedicated CSS class. (There isn't one for the "threads" button, for example.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look into this and try to implement your suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

@manancodes have you had a chance to look at this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into it again. The other icons being used are imported from the vector-im/compound-design-tokens library. Which is why it does not require any additional css.
Screenshot 2023-11-07 at 8 00 34 PM

And the chat icon present there is an outline icon, which does not go well with the other icons.
https://github.com/vector-im/compound-design-tokens/blob/main/icons/chat.svg

In the legacy header, the SVG is used as a mask and background color property is added to the icon.

matrix-react-sdk/res/css/views/rooms/_LegacyRoomHeader.pcss

Line 187 in 6ae7c03

background-color: $icon-button-color;

So I used that Icon and applied css to have dynamic colors according to the selected theme. What is the right way to tackle this? Should the filled chat icon be added in the Library? What is the procedure for that?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. Yes, it definitely looks like we should be using a Compound design icon here, since all the other buttons in the header use one. And you're right that there only seems to be an outline icon there at the moment.

@kerryarchibald as our Compound Champion, can you advise on the best approach here?

color: $icon-button-color;
}
18 changes: 18 additions & 0 deletions src/components/views/rooms/RoomHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ import RoomAvatar from "../avatars/RoomAvatar";
import { formatCount } from "../../../utils/FormattingUtils";
import RightPanelStore from "../../../stores/right-panel/RightPanelStore";
import { Linkify, topicToHtml } from "../../../HtmlUtils";
import { Icon as ChatIcon } from "../../../../res/img/element-icons/feedback.svg";
import { isVideoRoom as calcIsVideoRoom } from "../../../utils/video-rooms";

/**
* A helper to transform a notification color to the what the Compound Icon Button
Expand All @@ -74,6 +76,7 @@ export default function RoomHeader({ room }: { room: Room }): JSX.Element {
const memberCount = useRoomMemberCount(room, { throttleWait: 2500 });

const { voiceCallDisabledReason, voiceCallClick, videoCallDisabledReason, videoCallClick } = useRoomCall(room);
const isVideoRoom = useFeatureEnabled("feature_video_rooms") && calcIsVideoRoom(room);
richvdh marked this conversation as resolved.
Show resolved Hide resolved

const groupCallsEnabled = useFeatureEnabled("feature_group_calls");
/**
Expand Down Expand Up @@ -189,6 +192,21 @@ export default function RoomHeader({ room }: { room: Room }): JSX.Element {
<VideoCallIcon />
</IconButton>
</Tooltip>
{/* Ensure that the chat button is only visible in video rooms */}
{isVideoRoom && (
<Tooltip label={_t("right_panel|video_room_chat|title")}>
<IconButton
// indicator={notificationColorToIndicator(threadNotifications)} TODO: This still needs work
richvdh marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

do you plan to implement this as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But I need some help with this part. I'm not able to figure out what to pass in this argument.

Copy link
Member

Choose a reason for hiding this comment

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

ok leave it for now and we can open a follow-up issue.

onClick={(evt) => {
evt.stopPropagation();
RightPanelStore.instance.showOrHidePanel(RightPanelPhases.Timeline);
}}
aria-label={_t("right_panel|video_room_chat|title")}
>
<ChatIcon className="mx_RoomHeader_ChatIcon" />
</IconButton>
</Tooltip>
)}
<Tooltip label={_t("common|threads")}>
<IconButton
indicator={notificationColorToIndicator(threadNotifications)}
Expand Down