Unified grid layout #2325
Unified grid layout #2325
Conversation
Here I've implemented an MVP for the new unified grid layout, which scales smoothly up to arbitrarily many participants. It doesn't yet have a special 1:1 layout, so in spotlight mode and 1:1s, we will still fall back to the legacy grid systems. Things that happened along the way: - The part of VideoTile that is common to both spotlight and grid tiles, I refactored into MediaView - VideoTile renamed to GridTile - Added SpotlightTile for the new, glassy spotlight designs - NewVideoGrid renamed to Grid, and refactored to be even more generic - I extracted the media name logic into a custom React hook - Deleted the BigGrid experiment
| const SpotlightTileView = useMemo( | ||
| () => | ||
| forwardRef<HTMLDivElement, TileProps<MediaViewModel[], HTMLDivElement>>( | ||
| function SpotlightTileView( |
There was a problem hiding this comment.
Nitpick, I think this can be an anonymous function
| function SpotlightTileView( | |
| function ( |
There was a problem hiding this comment.
It can't unfortunately, because then the component will be missing a display name for React devtools (which the linter rightfully complains about)
| items: TileDescriptor<T>[]; | ||
| layout: Layout; | ||
| disableAnimations: boolean; | ||
| layoutStates: LayoutStatesMap; | ||
| children: (props: ChildrenProperties<T>) => ReactNode; | ||
| Tile: ComponentType<TileProps<T, R>>; |
There was a problem hiding this comment.
Nitpick, should Tile be lower-cased when it's a field name? This would also avoid confusion with the Tile type on line 58.
There was a problem hiding this comment.
It seems to be a React convention to capitalize names that refer to components, even field names or prop names. Part of the reason is that a camel case name can't be used to refer to a component in JSX, or else React will think you're trying to reference an "intrinsic element" like div, button, etc.
| @@ -154,11 +154,11 @@ class UserMedia { | |||
| this.vm = new UserMediaViewModel(id, member, participant, callEncrypted); | |||
|
|
|||
| this.speaker = this.vm.speaking.pipeState( | |||
| // Require 1 s of continuous speaking to become a speaker, and 10 s of | |||
| // Require 1 s of continuous speaking to become a speaker, and 60 s of | |||
There was a problem hiding this comment.
Was 10 seconds really too short? 60 feels like a lot
There was a problem hiding this comment.
It did seem to be too short in practice. In our team meetings for example, speakers often hold the floor for 30s at a time. I don't think we're at risk of the speaker section becoming overly large in large calls, because the number of people that speak during any given minute has a somewhat low upper limit - and I do think 60s has been feeling pretty good IMO!
Here I've implemented an MVP for the new unified grid layout, which scales smoothly up to arbitrarily many participants. It doesn't yet have a special 1:1 layout, so in spotlight mode and 1:1s, we will still fall back to the legacy grid systems.
Things that happened along the way:
Closes #1613
Closes #1203
Closes #1210
Closes #1154
Closes #1133
Closes #597