[bugfix] Mobile configuration page enable devices button when camera or microphone is available#3944
Conversation
edwardlee-msft
requested review from
AmyL219,
JamesBurnside,
JoshuaLai,
Leah-Xia-Microsoft,
PorterNan,
alkwa-msft,
carocao-msft,
dmceachernmsft,
emlynmac,
jimchou-dev,
jpeng-ms,
mgamis-msft,
palatter,
prabhjot-msft,
vhuseinova-msft and
waynemo
CallWithChat bundle size is increased❗.
|
Chat bundle size is not changed.
|
Calling bundle size is increased❗.
|
| data-ui-id="call-composite-local-device-settings-options-button" | ||
| {...devicesButtonProps} | ||
| // disable button whilst all other buttons are disabled | ||
| disabled={!microphonePermissionGranted || !cameraPermissionGranted || hasNoDevices} |
There was a problem hiding this comment.
I suppose technically we should be checking both the permission cross referencing those if there are devices for each?
const hasNoMics = !microphonePermissionGranted || !devicesButtonProps.microphones.length;
const hasNoSpeakers = !devicesButtonProps.speakers.length; // any permission needed here?
const hasNoCamera = !cameraPermissionGranted || !devicesButtonProps.cameras.length;
disabled = hasNoMics && hasNoSpeakers && hasNoCamera;There was a problem hiding this comment.
Thinking of the case where we have cameras, but no camera permission
There was a problem hiding this comment.
Although no permission maybe means we have no cameras... Though not sure if no camera via the role capability means that
There was a problem hiding this comment.
Actually yeah no we just need to check the camera permission - ignore all this ^_^
There was a problem hiding this comment.
Reopening -- but only because we might have no camera permission and no mic permission -- but still want to change the speaker.
There was a problem hiding this comment.
Thinking of the case where we have cameras, but no camera permission
No! I was thinking of the case where we have camera permission, but no cameras and we have microphones but no microphone permission (and lets assume no speakers). Then the button won't be disabled when it should be
There was a problem hiding this comment.
Can you tell its the Friday before xmas 🎅
There was a problem hiding this comment.
I like this, as it does a check for when devices have no microphone or camera present despite having permissions.
However, the hasNoDevices does something similar here for this specific case so we are covered I believe.
There was a problem hiding this comment.
Looks like there's still a couple edge cases:
- no camera or microphone permission but user wishes to change speaker (button will be disabled when it should not)
- we have camera permission and no cameras, and we have no microphone permission but do have mics, and have no speakers (button will be enabled when it should not)
@azure/communication-react jest test coverage for beta.
|
@azure/communication-react jest test coverage for stable.
|
edwardlee-msft
deleted the
elee/bugfix-mobile-devices-button-config-page
branch
3 participants
What
Enable devices button on localpreview on mobile configuration page when either microphone or camera is available
Why
https://skype.visualstudio.com/SPOOL/_workitems/edit/3520771
How Tested
MacOS - Chrome - Calling Sample - Mobile view
Process & policy checklist
Is this a breaking change?