Add room topic and animation #11352
Add room topic and animation #11352
Conversation
Mark RoomName component as deprecated
germain-gg
added
the
T-Task
|
@germain-gg any chance of a screenshot with a multi-line topic? Like the one in Element Web/Desktop 
|
|
@germain-gg thanks, is it intentional that links aren't linkified? |
|
yes |
|
Cool, liking the animation. Should we be concerned about prefers-reduced-motion at all? |
|
Probably yes, that's a good shout. @t3chguy wanna give this pull request a review? |
|
@germain-gg any chance it can meet the sonar gate so it doesn't need to bypass the merge queue? |
| : rightPanel.setCard({ phase: RightPanelPhases.RoomSummary }); | ||
| }} | ||
| > | ||
| {room && <DecoratedRoomAvatar room={room} oobData={oobData} avatarSize={40} displayBadge={false} />} |
There was a problem hiding this comment.
We want an avatar built from oobData if no room is present RoomAvatar doesn't require a room and will do just that.
There was a problem hiding this comment.
DecoratedRoomAvatar requires a room model. I'm not sure how that would work?
The entire component runs under the assumption this model exists and is passed down
There was a problem hiding this comment.
Switch between the two components depending on whether or not you have a room or make DecoratedRoomAvatar's room prop optional like RoomAvatar? The whole point of oobData.avatarUrl is to be able to render an avatar even if you don't have a room object
There was a problem hiding this comment.
Either that or make room required like LegacyRoomHeader
There was a problem hiding this comment.
So this behaviour is currently not possible. I've added a task to deal with that element-hq/element-web#25902 but this is probably not the right pull request to deal with it.
There was a problem hiding this comment.
| {room && <DecoratedRoomAvatar room={room} oobData={oobData} avatarSize={40} displayBadge={false} />} | |
| {room ? <DecoratedRoomAvatar room={room} oobData={oobData} avatarSize={40} displayBadge={false} /> : <RoomAvatar oobData={oobData} width={40} height={40} />} |
Switch between the two components depending on whether or not you have a room
Is very much possible
Either that or make room required like LegacyRoomHeader
As is this, it isn't at all clear why room is optional
There was a problem hiding this comment.
I don't think we should embed the logic here. But it is done so that we can move on and merge this pull request.
Room is marked as optional because it is the reality of the types. LegacyRoomHeader types are wrong.
The room object is passed from the RoomContext and the room property is Room | undefined there. I have not spent the time investigating nor decided how i'm going to fix it properly moving forward.
There was a problem hiding this comment.
The primary call site of LegacyRoomHeader is in RoomView, this is guarded by this block which ensures that a falsey room is not passed
Typescript reflects this type narrowing
The call site in WaitingForThirdPartyRoomView is similarly guarded though indirectly
The one in LocalRoomView is type asserted but then room.isError is accessed so that would throw before RoomHeader, again causing narrowing
The only one breaking assertions is LocalRoomCreateLoader
There was a problem hiding this comment.
Sounds good. I've pushed your suggestion, so we should probably keep it like this for this pull request to not have other matters creep in.
Unless you have other suggestions, we should be good to go?
( #11352 (comment) ) That is going to break probably more than 70% of the matrix room topics. Most projects use it to link to their github, website and so on. Not being able to linkify is a major downgrade and probably not well accepted when it is launched :/ It makes the already fairly useless topics even less useful. (useless in the sense that it currently has a very limited amount of info it can carry) |
For element-hq/element-web#25892
Screen.Recording.2023-08-01.at.13.27.39.mov
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.