Mv 286 limit displayed video tiles #132
Conversation
pkrucz00
force-pushed
the
MV-286-limit-displayed-video-tiles
branch
from
1835058 to
4f1fff7
Compare
| export const mapOtherToElement = ({ initialsFront, initialsBack, noLeftUsers }: OthersTileConfig): JSX.Element => ( | ||
| <OthersTile key="others" initialsFront={initialsFront} initialsBack={initialsBack} numberOfLeftTiles={noLeftUsers} /> | ||
| ); |
There was a problem hiding this comment.
- This function is actually a component. It gets some props and returns JSX.Element
- key="other" is unnecessary because it is not a list
- It is a simple wrapper over OtherTile. You could remove the
keyattribute and then inline this wrapper.
| @@ -42,41 +43,46 @@ type Props = { | |||
| const PinnedTilesSection: FC<Props> = ({ pinnedTiles, unpin, webrtc, showSimulcast }: Props) => { | |||
| const gridConfig = getGridConfig(pinnedTiles.length); | |||
|
|
|||
| const mapMediaTileConfigToElement = (pinnedTile: MediaPlayerTileConfig) => { | |||
There was a problem hiding this comment.
- It's a valid component, it gets some props and returns JSX
- I would suggest extracting it outside current component
2.a Have a look: https://www.youtube.com/watch?v=2sAdzy90GtE
2.b It's easier to reason about functional components declared as separate functions than this one because it's not clear which parameters are required. For now, it's a closure that have reference to gridConfig, unpin, showSimulcast and webrtc.
There was a problem hiding this comment.
What about making a component PinnedTile with all the properties outside of the PinnedTilesSection component but with PartialPinnedTile nested component that uses closure and only takes MediaTileConfig as an argument? This should clear the code out of unnecessary props drowning while not allowing new bugs to appear 🐞
There was a problem hiding this comment.
Ad. 2a - isn't this important only if the component has it's own independent state? If it's a simple "map" like the example above, I don't think the placement of the component makes any difference.
| @@ -48,43 +48,48 @@ const UnpinnedTilesSection: FC<Props> = ({ | |||
|
|
|||
| const tileSize = tileConfigs.length >= 7 ? "M" : "L"; | |||
|
|
|||
| const mapMediaTileConfigToElement = (config: MediaPlayerTileConfig): JSX.Element => { | |||
There was a problem hiding this comment.
same as mapMediaTileConfigToElement in PinnedTilesSection.tsx
|
Converted to draft as it is blocked by MV-288 |
|
Closed in favour of #182 |
No description provided.