-
-
Notifications
You must be signed in to change notification settings - Fork 828
Move voip previews to bottom right corner #4904
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.
Please in the future do not mix TypeScript conversions with functional changes, as it's hard to review. It's made even more difficult to review when all of them are in a single commit.
I've done an initial pass here and it overall seems reasonable, though the tests are super angry.
SettingsStore.watchSetting("feature_new_room_list", null, (name, roomId, level, valAtLevel, newVal) => this.setState({ | ||
newRoomListActive: newVal, | ||
})); |
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 needs an unwatch too
src/settings/Settings.js
Outdated
@@ -305,7 +305,7 @@ export const SETTINGS = { | |||
"VideoView.flipVideoHorizontally": { | |||
supportedLevels: LEVELS_ACCOUNT_SETTINGS, | |||
displayName: _td('Mirror local video feed'), | |||
default: false, | |||
default: true, |
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 requires a product decision, I think.
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.
How do I go about getting one?
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.
making noise in the direction of Nad
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.
ftr: element-hq/element-web#10651 and element-hq/element-web#5633 are relevant issues.
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.
@nadonomy for context the current 1:1 video calls have a small local video feed. This feed is not mirrored by default although I think it aught to be. (See the issues linked by turt2live)
Currently it looks like this:
But it's probably a lot more intuitive like this:
Should we make the default mirrored?
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.
Per the call: we'll keep this off by default for now as the screen sharing issue needs work.
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.
Because of element-hq/element-web#5633 we're not doing this but I'm making it my personal mission to fix that one soon.
… joriks/room-list-voip
I thought it would be a small conversion but it got out of hand. I'll be more segmented about it in future. |
- spelling mistake - unwatch watched setting - lint (indentation) - use more performant component
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.
lgtm assuming product is okay with the change
Just to summarise some internal 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.
good to merge once the mirror flip is flipped
fixes: element-hq/element-web#14177
relates to: element-hq/element-web#13635 in PRs for overarching tracking
There is also a video view: