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

Show speaker in the spotlight in large grids #2416

Merged

Conversation

robintown
Copy link
Member

Based on #2382

@@ -504,7 +504,9 @@ export class CallViewModel extends ViewModel {
(grid, spotlight, screenShares): Layout => ({
type: "grid",
spotlight:
screenShares.length > 0 ? spotlight : undefined,
screenShares.length > 0 || grid.length > 20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 20 seems somewhat random. I think this should either become a config or needs a comment what made us decide to hard code this to 20.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe also a "needs to show scrollbars" is the better condition?

Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best to use a non hardcoded condition here.

If we use the 20 limit as a stop gap is also okay. But then I think it would be nice to have comments explaining and justifying it (maybe even mentioning that this could be done with a "do we need scrollbars" condition

@robintown
Copy link
Member Author

I think I do have an argument against basing the decision on whether there is a scroll bar:

  • Speakers are always promoted to the start of the grid, so it should be exceedingly rare that a speaker stays outside the visible area.
  • So then, the purpose of this PR is not so much to bring obscured speakers into view, but rather to reduce the cognitive load of finding the speaker in a large grid of people.
  • I think it's reasonable to assume that cognitive load is correlated to the number of participants in a call, but not necessarily to whether there is a scroll bar. (For example, if I have a large window showing 30 participants without a scroll bar, and then I make the window smaller so that it has a scroll bar and only shows the most important 15 participants, has the cognitive load really increased? I think it's gone down, if anything.)
  • So, I think using a heuristic based on number of participants is appropriate.

However I totally get the desire to not use an arbitrary number. Basing the decision on whether there is a scroll bar would be pretty non-trivial, and this part of the designs is still subject to change (we want to workshop it further), so: I suggest that the scroll bar method is not worth the time investment at this stage, regardless of whether my argument above makes sense.

@toger5
Copy link
Contributor

toger5 commented Jul 18, 2024

I suggest that the scroll bar method is not worth the time investment at this stage, regardless of whether my argument above makes sense.

I get behind this!
I was mostly thinking that the reason for this is to always have the speaker visible. With more ppl (or a scroll container) there is the chance (independent of the user being at the bottom or the top) to not see a user so it makes sense to at least make sure the speaker is visible.

But I like going with this first and making a decision that requires a lot of dev effort based on more usage experience!

@robintown robintown merged commit d561a41 into element-hq:new-call-layouts Jul 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants