Skip to content
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

Separate layout's logic from ranking logic #1133

Closed
daniel-abramov opened this issue Jun 22, 2023 · 1 comment
Closed

Separate layout's logic from ranking logic #1133

daniel-abramov opened this issue Jun 22, 2023 · 1 comment
Labels
A-Developer-Experience Workflow of developing: building, linting, debugging, profiling, etc. A-Spotlight Spotlight layout, where the active speaker is foregrounded T-Task Refactoring, enabling or disabling functionality, other engineering tasks

Comments

@daniel-abramov
Copy link
Contributor

daniel-abramov commented Jun 22, 2023

Currently, the grid layout has too many responsibilities that make changes hard and increase the likelihood of bugs.

We need to separate the rendering/animation logic from the logic that orders the tiles (decides on the ranking of elements).

See #1092 (comment) for more details.

@robintown, here I would like to describe a rough idea of how I think we could refactor it to make it better:

  1. I think it would be amazing if we could make the video grid only care about the rendering and animation logic so that the grid layout does not need to rank/order the tiles on its own. It would be a single responsibility - get the input items (tile descriptions) and render them (animating them accordingly if the previously sent items had a different order). That way a tile description will be as simple as a tiny structure with an id, rank (or order), and highlight (for speaking indicators).
  2. Ranking of tiles will be done in a separate component (as it's purely algorithmic). That component will get the list of participants with their properties as input and rank them based on what is expected to be shown in the GUI, passing the resulting list of tile descriptors to the grid layout.

This way we not only decouple two concepts from each other allowing different people to work on different parts without knowing the implementation detail on the other part but also potentially fixing bugs as the responsibilities of each component are clearly defined and there are no implicit expectations from the caller regarding how the render will re-order the tiles.

We also gain more flexibility as we don't need to change anything in the grid layout whenever we want to change the order in which the participants are displayed or highlighted.

So in this case the input to the grid layout will be:

  1. The list of tile descriptors.
  2. The type of layout (unless we decide that layouts are different and that we want to have 2 different layouts, we can discuss that).
  3. On click handlers. That way the layout will not "promote" the participant that has been double-clicked, instead we let the caller decide what to do when the participant is double-clicked (also would allow to implement the pinning logic in a more elegant way for the spotlight).
@daniel-abramov daniel-abramov added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Jun 22, 2023
@robintown robintown added T-Task Refactoring, enabling or disabling functionality, other engineering tasks A-Spotlight Spotlight layout, where the active speaker is foregrounded A-Developer-Experience Workflow of developing: building, linting, debugging, profiling, etc. and removed T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements labels Jun 27, 2023
@robintown
Copy link
Member

#2325

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Developer-Experience Workflow of developing: building, linting, debugging, profiling, etc. A-Spotlight Spotlight layout, where the active speaker is foregrounded T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
No open projects
Development

No branches or pull requests

2 participants