-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
together mode component #5517
base: main
Are you sure you want to change the base?
together mode component #5517
Conversation
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. |
1 similar comment
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. |
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-crsepazbre.chromatic.com/ |
@azure/communication-react jest test coverage for stable.
|
@azure/communication-react jest test coverage for beta.
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-cnekgiwzhv.chromatic.com/ |
packages/react-components/src/components/TogetherModeOverlay.tsx
Outdated
Show resolved
Hide resolved
packages/react-components/src/components/TogetherModeOverlay.tsx
Outdated
Show resolved
Hide resolved
packages/react-components/src/components/TogetherModeOverlay.tsx
Outdated
Show resolved
Hide resolved
packages/react-components/src/components/VideoGallery/TogetherModeLayout.tsx
Outdated
Show resolved
Hide resolved
packages/react-components/src/components/VideoGallery/TogetherModeStream.tsx
Show resolved
Hide resolved
packages/react-components/src/components/VideoGallery/TogetherModeStream.tsx
Show resolved
Hide resolved
packages/react-components/src/components/VideoGallery/TogetherModeStream.tsx
Outdated
Show resolved
Hide resolved
packages/react-components/src/components/styles/TogetherMode.styles.ts
Outdated
Show resolved
Hide resolved
packages/react-components/src/components/styles/TogetherMode.styles.ts
Outdated
Show resolved
Hide resolved
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-swjbpdkbrv.chromatic.com/ |
CallWithChat bundle size is not changed.
|
Calling bundle size is not changed.
|
Chat bundle size is decreased✅.
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-rqqrajzgjk.chromatic.com/ |
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-niicbykrep.chromatic.com/ |
* | ||
* @returns {JSX.Element} An empty JSX element. | ||
*/ | ||
export const TogetherModeOverlay = React.memo( |
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.
can we import memo
from React here? or do we need to use the keyword? we want to avoid this if we can to make sure we are using the ESM module
<div | ||
style={moveAnimationStyles( | ||
parseFloat(participantStatus.seatPositionStyle.seatPosition.height) * 0.5, | ||
parseFloat(participantStatus.seatPositionStyle.seatPosition.height) * 0.35 |
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.
Can we get some constants to define these numbers? just so we can know what they do in the future if we need to update them
width: `${emojiSize}`, | ||
position: 'absolute', | ||
left: `${ | ||
(100 - |
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.
Same here with these numbers
parentWidth | ||
]); | ||
|
||
return screenShareComponent ? ( |
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.
To cut down on duplicated code maybe the thing to do here when screenshare is active is to have the VideoGallery use another layout. at lease until we are wanting to support a different view for together mode?
screenShareComponent?: JSX.Element; | ||
containerWidth?: number; | ||
containerHeight?: number; | ||
}): React.ReactNode => { |
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.
is there a reason that this is a ReactNode and not a JSX element?
/** | ||
* @private | ||
*/ | ||
export const togetherModeParticipantStatusContainer = ( |
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.
Can we have a majority of these styles in a separate style.
And then just have a merge styles instead for specific background color, radius?
Same thing for the togetherModeParticipantDisplayName
Edit: On second thought, we can probably keep it like this if there is no reusuable code
* @returns The scaled size. | ||
*/ | ||
export const calculateScaledSize = (width: number, height: number): number => { | ||
const maxSize = 600; |
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.
What do these numbers mean? What is maxSize and what is minSize?
Should these just be constants at the top of the page?
What
This PR contains together mode component layout and styling
Why
Needed as part of together mode implementation
How Tested
locally
Process & policy checklist
Is this a breaking change?