[Custom Layouts] Speaker gallery component #3443
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "type": "prerelease", | ||
| "area": "feature", | ||
| "workstream": "Custom Layouts", | ||
| "comment": "Introduces Speaker gallery layout to the video gallery component", | ||
| "packageName": "@azure/communication-react", | ||
| "email": "94866715+dmceachernmsft@users.noreply.github.com", | ||
| "dependentChangeType": "patch" | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "type": "prerelease", | ||
| "area": "feature", | ||
| "workstream": "Custom Layouts", | ||
| "comment": "Introduces Speaker gallery layout to the video gallery component", | ||
| "packageName": "@azure/communication-react", | ||
| "email": "94866715+dmceachernmsft@users.noreply.github.com", | ||
| "dependentChangeType": "patch" | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,249 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| import { LayerHost, Stack, mergeStyles, useTheme } from '@fluentui/react'; | ||
| /* @conditional-compile-remove(click-to-call) */ | ||
| import { LocalVideoTileSize } from '../VideoGallery'; | ||
| import { LayoutProps } from './Layout'; | ||
| import { isNarrowWidth } from '../utils/responsive'; | ||
| /* @conditional-compile-remove(vertical-gallery) */ | ||
| import { isShortHeight } from '../utils/responsive'; | ||
| import React, { useMemo, useRef, useState } from 'react'; | ||
| import { OverflowGallery } from './OverflowGallery'; | ||
| import { | ||
| SMALL_FLOATING_MODAL_SIZE_REM, | ||
| LARGE_FLOATING_MODAL_SIZE_REM, | ||
| localVideoTileContainerStyle | ||
| } from './styles/FloatingLocalVideo.styles'; | ||
| /* @conditional-compile-remove(vertical-gallery) */ | ||
| import { | ||
| VERTICAL_GALLERY_FLOATING_MODAL_SIZE_REM, | ||
| SHORT_VERTICAL_GALLERY_FLOATING_MODAL_SIZE_REM | ||
| } from './styles/FloatingLocalVideo.styles'; | ||
| import { useOrganizedParticipants } from './utils/videoGalleryLayoutUtils'; | ||
| import { GridLayout } from '../GridLayout'; | ||
| import { rootLayoutStyle } from './styles/FloatingLocalVideoLayout.styles'; | ||
| import { layerHostStyle, innerLayoutStyle } from './styles/FloatingLocalVideoLayout.styles'; | ||
| import { videoGalleryLayoutGap } from './styles/Layout.styles'; | ||
| import { useId } from '@fluentui/react-hooks'; | ||
|
|
||
| /** | ||
| * Props for {@link FloatingLocalVideoLayout}. | ||
| * | ||
| * @private | ||
| */ | ||
| export interface SpeakerVideoLayoutProps extends LayoutProps { | ||
| /** | ||
| * Whether to display the local video camera switcher button | ||
| */ | ||
| showCameraSwitcherInLocalPreview?: boolean; | ||
| /** | ||
| * Height of parent element | ||
| */ | ||
| parentHeight?: number; | ||
| /* @conditional-compile-remove(click-to-call) */ | ||
| /** | ||
| * Local video tile mode | ||
| */ | ||
| localVideoTileSize?: LocalVideoTileSize; | ||
| } | ||
|
|
||
| /** | ||
| * Layout for the gallery mode to highlight the current dominant speaker | ||
| * | ||
| * @private | ||
| */ | ||
| export const SpeakerVideoLayout = (props: SpeakerVideoLayoutProps): JSX.Element => { | ||
| const { | ||
| remoteParticipants = [], | ||
| dominantSpeakers, | ||
| localVideoComponent, | ||
| screenShareComponent, | ||
| onRenderRemoteParticipant, | ||
| styles, | ||
| maxRemoteVideoStreams, | ||
| parentWidth, | ||
| /* @conditional-compile-remove(vertical-gallery) */ parentHeight, | ||
| /* @conditional-compile-remove(vertical-gallery) */ overflowGalleryPosition = 'HorizontalBottom', | ||
| pinnedParticipantUserIds = [], | ||
| /* @conditional-compile-remove(click-to-call) */ localVideoTileSize | ||
| } = props; | ||
|
|
||
| const theme = useTheme(); | ||
|
|
||
| const isNarrow = parentWidth ? isNarrowWidth(parentWidth) : false; | ||
|
|
||
| /* @conditional-compile-remove(vertical-gallery) */ | ||
| const isShort = parentHeight ? isShortHeight(parentHeight) : false; | ||
|
|
||
| // This is for tracking the number of children in the first page of overflow gallery. | ||
| // This number will be used for the maxOverflowGalleryDominantSpeakers when organizing the remote participants. | ||
| const childrenPerPage = useRef(4); | ||
| const { gridParticipants, overflowGalleryParticipants } = useOrganizedParticipants({ | ||
| remoteParticipants, | ||
| dominantSpeakers, | ||
| maxRemoteVideoStreams, | ||
| isScreenShareActive: !!screenShareComponent, | ||
| maxOverflowGalleryDominantSpeakers: screenShareComponent | ||
| ? childrenPerPage.current - (pinnedParticipantUserIds.length % childrenPerPage.current) | ||
| : childrenPerPage.current, | ||
| /* @conditional-compile-remove(pinned-participants) */ pinnedParticipantUserIds, | ||
| /* @conditional-compile-remove(gallery-layouts) */ layout: 'speaker' | ||
| }); | ||
|
|
||
| let activeVideoStreams = 0; | ||
|
|
||
| const gridTiles = gridParticipants.map((p) => { | ||
| return onRenderRemoteParticipant( | ||
| p, | ||
| maxRemoteVideoStreams && maxRemoteVideoStreams >= 0 | ||
| ? p.videoStream?.isAvailable && activeVideoStreams++ < maxRemoteVideoStreams | ||
| : p.videoStream?.isAvailable | ||
| ); | ||
| }); | ||
|
|
||
| const shouldFloatLocalVideo = remoteParticipants.length > 0; | ||
|
Member
There was a problem hiding this comment. Concerned there is so much duplicate logic between these different layouts (and we're adding more layout(s)). It increases the chance there's a bug with just one layout that doesn't appear in others and makes it really hard to add a feature to all layouts. (it also makes it very hard to review as there isn't a good way to compare this layout with other layouts without having the up side by side) Not in this PR, but strongly consider refactoring how we structure these layouts. Ideally we split the business logic (of deciding what tile should be shown where) with the UI logic (the actual rendering of the tiles). const NewGalleryLayout = props => {
const tileDefinitions = getTileDefinitions('newLayout', localParticipant, remoteParticipants, dominantSpeakers, height, width, etc.);
// returns something like:
// {
// primaryGridTiles: [remoteParticipantId, remoteParticipantId, remoteParticipantId],
// overflowTiles: [...],
// floatingTile: [],
// dockedCornerTile: [localParticipant]
// }
// then have memoized tile factories that return JSX elements that are the tiles:
const primaryTiles = buildPrimaryTiles(tileDefinitions, renderOverrides, etc...);
const overflowTiles = getOverflowTiles(tileDefinitions, renderOverrides, etc...);
const floatingTile .. etc.etc.
// Then have a single gallery with the places to put these:
<VideoGalleryBase
primaryTiles={primaryTiles}
overflowTiles={overflowTiles}
floatingTile={floatingTile}
etc.etc.
/>
}i.e. have getTiles(constraints); // just state where this is business logic that is isolated and easily unit tested
buildTiles(tileDefinitions) // just constructing JSX where the tiles can be memoized and reused when layouts switch
<Gallery {...tiles} /> // just positions tiles it receives appropriately, completely layout agnosticJust thinking of this while typing so not a perfect solution, but something where we can split UI and tile logic, then easily unit test the tile logic with differing constraints. If we ever did go down this route we could utilize a component specific provider to store tiles such that they don't get recreated for different layouts as well where tiles get memoized and then if they're already built the buildXTiles method just grabs a memoized tile without having to construct a new one. |
||
|
|
||
| if (!shouldFloatLocalVideo && localVideoComponent) { | ||
| gridTiles.push(localVideoComponent); | ||
| } | ||
|
|
||
| /** | ||
| * instantiate indexes available to render with indexes available that would be on first page | ||
| * | ||
| * For some components which do not strictly follow the order of the array, we might | ||
| * re-render the initial tiles -> dispose them -> create new tiles, we need to take care of | ||
| * this case when those components are here | ||
| */ | ||
| const [indexesToRender, setIndexesToRender] = useState<number[]>([]); | ||
|
|
||
| const overflowGalleryTiles = overflowGalleryParticipants.map((p, i) => { | ||
| return onRenderRemoteParticipant( | ||
| p, | ||
| maxRemoteVideoStreams && maxRemoteVideoStreams >= 0 | ||
| ? p.videoStream?.isAvailable && | ||
| indexesToRender && | ||
| indexesToRender.includes(i) && | ||
| activeVideoStreams++ < maxRemoteVideoStreams | ||
| : p.videoStream?.isAvailable | ||
| ); | ||
| }); | ||
|
|
||
| const layerHostId = useId('layerhost'); | ||
|
|
||
| const localVideoSizeRem = useMemo(() => { | ||
| if (isNarrow || /*@conditional-compile-remove(click-to-call) */ localVideoTileSize === '9:16') { | ||
| return SMALL_FLOATING_MODAL_SIZE_REM; | ||
| } | ||
| /* @conditional-compile-remove(vertical-gallery) */ | ||
| if ((overflowGalleryTiles.length > 0 || screenShareComponent) && overflowGalleryPosition === 'VerticalRight') { | ||
| return isNarrow | ||
| ? SMALL_FLOATING_MODAL_SIZE_REM | ||
| : isShort | ||
| ? SHORT_VERTICAL_GALLERY_FLOATING_MODAL_SIZE_REM | ||
| : VERTICAL_GALLERY_FLOATING_MODAL_SIZE_REM; | ||
| } | ||
| /*@conditional-compile-remove(click-to-call) */ | ||
| if ((overflowGalleryTiles.length > 0 || screenShareComponent) && overflowGalleryPosition === 'HorizontalBottom') { | ||
| return localVideoTileSize === '16:9' || !isNarrow ? LARGE_FLOATING_MODAL_SIZE_REM : SMALL_FLOATING_MODAL_SIZE_REM; | ||
| } | ||
| return LARGE_FLOATING_MODAL_SIZE_REM; | ||
| }, [ | ||
| overflowGalleryTiles.length, | ||
| isNarrow, | ||
| screenShareComponent, | ||
| /* @conditional-compile-remove(vertical-gallery) */ isShort, | ||
| /* @conditional-compile-remove(vertical-gallery) */ overflowGalleryPosition, | ||
| /* @conditional-compile-remove(click-to-call) */ localVideoTileSize | ||
| ]); | ||
|
|
||
| const wrappedLocalVideoComponent = | ||
| localVideoComponent || (screenShareComponent && localVideoComponent) ? ( | ||
| <Stack | ||
| className={mergeStyles( | ||
| localVideoTileContainerStyle( | ||
| theme, | ||
| localVideoSizeRem, | ||
| !!screenShareComponent, | ||
| /* @conditional-compile-remove(gallery-layouts) */ overflowGalleryPosition | ||
| ) | ||
| )} | ||
| > | ||
| {localVideoComponent} | ||
| </Stack> | ||
| ) : undefined; | ||
|
|
||
| const overflowGallery = useMemo(() => { | ||
| if (overflowGalleryTiles.length === 0 && !screenShareComponent) { | ||
| return null; | ||
| } | ||
| return ( | ||
| <OverflowGallery | ||
| /* @conditional-compile-remove(vertical-gallery) */ | ||
| isShort={isShort} | ||
| onFetchTilesToRender={setIndexesToRender} | ||
| isNarrow={isNarrow} | ||
| shouldFloatLocalVideo={true} | ||
| overflowGalleryElements={overflowGalleryTiles} | ||
| horizontalGalleryStyles={styles?.horizontalGallery} | ||
| /* @conditional-compile-remove(vertical-gallery) */ | ||
| verticalGalleryStyles={styles?.verticalGallery} | ||
| /* @conditional-compile-remove(vertical-gallery) */ | ||
| overflowGalleryPosition={overflowGalleryPosition} | ||
| onChildrenPerPageChange={(n: number) => { | ||
| childrenPerPage.current = n; | ||
| }} | ||
| /> | ||
| ); | ||
| }, [ | ||
| isNarrow, | ||
| /* @conditional-compile-remove(vertical-gallery) */ isShort, | ||
| screenShareComponent, | ||
| overflowGalleryTiles, | ||
| styles?.horizontalGallery, | ||
| /* @conditional-compile-remove(vertical-gallery) */ overflowGalleryPosition, | ||
| setIndexesToRender, | ||
| /* @conditional-compile-remove(vertical-gallery) */ styles?.verticalGallery | ||
| ]); | ||
|
|
||
| return ( | ||
| <Stack styles={rootLayoutStyle}> | ||
| {wrappedLocalVideoComponent} | ||
| <LayerHost id={layerHostId} className={mergeStyles(layerHostStyle)} /> | ||
| <Stack | ||
| /* @conditional-compile-remove(vertical-gallery) */ | ||
| horizontal={overflowGalleryPosition === 'VerticalRight'} | ||
| styles={innerLayoutStyle} | ||
| tokens={videoGalleryLayoutGap} | ||
| > | ||
| { | ||
| /* @conditional-compile-remove(gallery-layouts) */ props.overflowGalleryPosition === 'HorizontalTop' ? ( | ||
| overflowGallery | ||
| ) : ( | ||
| <></> | ||
| ) | ||
| } | ||
| {screenShareComponent ? ( | ||
| screenShareComponent | ||
| ) : ( | ||
| <GridLayout key="grid-layout" styles={styles?.gridLayout}> | ||
| {gridTiles} | ||
| </GridLayout> | ||
| )} | ||
| {overflowGalleryTrampoline( | ||
| overflowGallery, | ||
| /* @conditional-compile-remove(gallery-layouts) */ props.overflowGalleryPosition | ||
| )} | ||
| </Stack> | ||
| </Stack> | ||
| ); | ||
| }; | ||
|
|
||
| const overflowGalleryTrampoline = ( | ||
| gallery: JSX.Element | null, | ||
| galleryPosition?: 'HorizontalBottom' | 'VerticalRight' | 'HorizontalTop' | ||
| ): JSX.Element | null => { | ||
| /* @conditional-compile-remove(gallery-layouts) */ | ||
| return galleryPosition !== 'HorizontalTop' ? gallery : <></>; | ||
| return gallery; | ||
| }; | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm totally fine with adding a layout prop to useOrganizedParticipants but I am curious if you can instead set maxRemoteVideoStreams as 0 and set remoteParticipants as
remoteParticipants.filter(p => userId !== dominantSpeaker[0])and gridTiles below (on line 97) would just beremoteParticipants.find(p => userId === dominantSpeaker[0]). Even if this works, it may be less intuitive for code readersThere 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.
That I think would work, but I like the use of the layout to do this determination, helps understand why we do or don't do something other than just the filtering of participants.