Fix bug where username disappears at 400 zoom #5730
Conversation
carocao-msft
requested review from
JamesBurnside,
alkwa-msft,
dmceachernmsft,
edwardlee-msft,
mgamis-msft,
prabhjot-msft,
jimchou-dev,
vhuseinova-msft,
Leah-Xia-Microsoft,
jpeng-ms,
waynemo,
emlynmac,
JoshuaLai and
palatter
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Failed to pass the composite UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-yoyzcgcrqq.chromatic.com/ |
@azure/communication-react jest test coverage for stable.
|
@azure/communication-react jest test coverage for beta.
|
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
| /** | ||
| * Boolean to indicate if it is mobile or desktop | ||
| */ | ||
| isMobileView?: boolean; |
There was a problem hiding this comment.
don't add isMobile to components. Component props should be specific and explicit about purpose, isMobile is a catch-all property that doesn't tell the developer what will happen to the component when applied, it also will impact multiple aspects of the component and has a great chance of conflicting with other properties. Instead have something like showNameplate and in the Composite layer do <VideoGallery showNameplate={isMobile} />
There was a problem hiding this comment.
What about showMobileVIew or showCompactView? Based on our discussion previously, we want to only show the video gallery compact view based on isMobile instead of determine based on width. #5643
There was a problem hiding this comment.
What precisely does compact view do? How many things change?
There was a problem hiding this comment.
For now it's showing label and having displayname on local video tile. In the future we might have other UI added for mobile video gallery, we want to start using the isMobile value to make sure when on dekstop we are forcing the videoGallery to not trip is narrow and to always be the desktop mode
There was a problem hiding this comment.
For those few items we should have individual properties for. What if a developer wants a narrow experience on desktop? Or for ipads/tablets that have larger screens are they isMobile? (They may want some ismobile properties like larger touch targets, or swipe-able overflow gallery, but not other ismobile properties that save space like hiding display name). When we bucket many different properties under a single property we remove flexibility for developers and only make our component better for the one experience we have in mind today.
There was a problem hiding this comment.
I started a group chat with you and Don to discuss this API further.
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
Calling bundle size is not changed.
|
Chat bundle size is increased❗.
|
CallWithChat bundle size is not changed.
|
|
Failed to pass the composite UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-pnpxpixdud.chromatic.com/ |
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
What
Fix bug where username disappears at 400 zoom
Why
At 400% zoom the width decreased into the isNarrow range, so mobile view is selected and username diappears.
Use isMobile to determine when to show mobile view instead of tracking width.
How Tested
Process & policy checklist
Is this a breaking change?