Fix: No way to open chat in video rooms with new header. #11752
Conversation
github-actions
bot
added
Z-Community-PR
|
Also, I will add the tests and update snapshots. But before that I would like somebody to review the implementation once. |
| * { | ||
| fill: $icon-button-color; | ||
| } |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
I will look into this and try to implement your suggestion.
There was a problem hiding this comment.
@manancodes have you had a chance to look at this?
There was a problem hiding this comment.
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.
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.
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?
There was a problem hiding this comment.
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?
| {isVideoRoom && ( | ||
| <Tooltip label={_t("right_panel|video_room_chat|title")}> | ||
| <IconButton | ||
| // indicator={notificationColorToIndicator(threadNotifications)} TODO: This still needs work |
There was a problem hiding this comment.
do you plan to implement this as part of this PR?
There was a problem hiding this comment.
Yes. But I need some help with this part. I'm not able to figure out what to pass in this argument.
There was a problem hiding this comment.
ok leave it for now and we can open a follow-up issue.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
| * { | ||
| fill: $icon-button-color; | ||
| } |
There was a problem hiding this comment.
I will look into this and try to implement your suggestion.
| {isVideoRoom && ( | ||
| <Tooltip label={_t("right_panel|video_room_chat|title")}> | ||
| <IconButton | ||
| // indicator={notificationColorToIndicator(threadNotifications)} TODO: This still needs work |
There was a problem hiding this comment.
Yes. But I need some help with this part. I'm not able to figure out what to pass in this argument.
|
@manancodes apart from the question about the icon, we could do with some tests for this change. Would you be able to add tests please? |
|
Yes. I'll write some tests for it. |
|
Hi @manancodes, a solid chat icon is available in the latest design tokens release https://github.com/vector-im/compound-design-tokens/releases/tag/v0.1.0 |
|
Thanks for your work on this @manancodes, this issue was fixed by #11911 |
Checklist
Fixes: element-hq/element-web#26181
Signed-off-by: Manan Sadana manancodes.dev@gmail.com
Before:
After:
Type: defect
Here's what your changelog entry will look like:
🐛 Bug Fixes