[Custom Layouts] Speaker gallery component#3443
Conversation
…hub.com/Azure/communication-ui-library into dmceachernmsft-custom-layout-playground
…hub.com/Azure/communication-ui-library into dmceachernmsft-custom-layout-playground
|
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. |
| ? childrenPerPage.current - (pinnedParticipantUserIds.length % childrenPerPage.current) | ||
| : childrenPerPage.current, | ||
| /* @conditional-compile-remove(pinned-participants) */ pinnedParticipantUserIds, | ||
| /* @conditional-compile-remove(gallery-layouts) */ layout: 'speaker' |
There was a problem hiding this comment.
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 be remoteParticipants.find(p => userId === dominantSpeaker[0]). Even if this works, it may be less intuitive for code readers
There was a problem hiding this comment.
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.
…com/Azure/communication-ui-library into dmceachernmsft/speaker-gallery-component
| ); | ||
| }); | ||
|
|
||
| const shouldFloatLocalVideo = remoteParticipants.length > 0; |
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).
One possibility would be to:
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.
| : []; | ||
| /* @conditional-compile-remove(gallery-layouts) */ | ||
| if (dominantSpeakerToGrid[0]) { | ||
| visibleGridParticipants.current = dominantSpeakerToGrid; |
There was a problem hiding this comment.
We set visibleGridParticipants.current on L61 then this overrides it.. based on dominantSpeakerToGrid having a valid 0th element? This is difficult to parse why and that these variables refer to.
dominantSpeakerToGrid I think implies one dominant speaker to be placed in the grid portion of the video gallery? But its an array so perhaps dominantSpeakers plural? Not convinced what the toGrid means 😅
Just trying to understand why we override this value
There was a problem hiding this comment.
Oh I see, for speaker layout, we want a single speaker to be the grid participant
There was a problem hiding this comment.
this is how we set the one participant that is the most dominant speaker, we could probably do it first but I wanted to have go against the people who would be there when the call first starts so we can have a default first person and reuse the screen share logic
There was a problem hiding this comment.
Can you add a comment to this effect?
Just something like
// When in speaker mode we want the visibleGridParticipants to be only the most prevelant speaker. This is either the most dominant speaker in the list of visibleGridParticipants or the first visibleGridParticipants
| }).slice(0, maxRemoteVideoStreams); | ||
|
|
||
| /* @conditional-compile-remove(gallery-layouts) */ | ||
| const dominantSpeakerToGrid = |
There was a problem hiding this comment.
Can we add any unit tests for this logic?
There was a problem hiding this comment.
yup in PR following it in
3 participants
What
Introduces new Speaker mode layout to the videoGallery
Why
Allows the user to focus on the most recent speaker
https://skype.visualstudio.com/SPOOL/_workitems/edit/3362646
https://skype.visualstudio.com/SPOOL/_workitems/edit/3362644
How Tested
Locally, E2E tests to come
