Conversation
There was a problem hiding this comment.
The code looks good, and things also work as written, but I think we should not merge this until we have Core the API to actually convey the info about whether this is a video or an audio call to the remote party, i.e. chatmail/core#7740. Because otherwise the button is kind of misleading. You think that you're starting an audio-only call, but for the callee the camera is enabled by default.
| startOutgoingVideoCall( | ||
| accountId: number, | ||
| chatId: number, | ||
| cameraEnabled: boolean |
There was a problem hiding this comment.
Also maybe it's better to stick to noOutgoingVideoInitially?
Edit: I mean the name
There was a problem hiding this comment.
noOutgoingVideoInitially sounds for me more confusing
would you agree on
startWithCameraEnabled ?
nicodh
force-pushed
the
add-audio-video-select
branch
from
a15ba78 to
06046b4
Compare
WofWca
left a comment
WofWca
left a comment
There was a problem hiding this comment.
The core upgrade issue has been addressed.
I tested this, the "video enabled initially" state is conveyed and respected.
Regarding the UI / UX: not sure if it's the best to have people click two times (maybe we should have two separate buttons presented right away, maybe one hidden behind the three-dot menu), but at least Signal utilizes the same approach, so this is good.
Why not a toggle button? One image is the camera with a red line/cross over it and the other state shows an enabled camera. Then you only need one click to toggle the state. |
|
I though about that a little, and the problem with that is that it basically introduces a "default" mode for calls, i.e. not requiring the user to make an explicit choice. But then again, I am not sure if it's good or not. You are still free to toggle the camera however you like after you have already started the call. |
|
I remember that these issues were alreday discussed. (at least live, maybe online too somewhere) |
3 participants
Follow up to #5967