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

Add toggle to always show yourself #2380

Merged
merged 6 commits into from
Jul 17, 2024

Conversation

robintown
Copy link
Member

Based on #2368

@robintown robintown requested a review from a team as a code owner May 16, 2024 17:57
@robintown robintown changed the base branch from livekit to new-call-layouts June 7, 2024 14:49
@robintown robintown changed the title Add toggles to pin or always show a tile Add toggle to always show self Jun 20, 2024
@robintown robintown changed the title Add toggle to always show self Add toggle to always show local user media Jun 20, 2024
@robintown robintown changed the title Add toggle to always show local user media Add toggle to always show yourself Jun 20, 2024
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.

This looks good. Thanks.
Can you help me understand the reason local an remote user are different classes instead of a property on the user tile?

Comment on lines 134 to 141
SelfAlwaysShown,
Presenters,
Speakers,
VideoAndAudio,
Video,
Audio,
NoMedia,
SelfEnd,
SelfNotAlwaysShown,
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not immediately translate to a bin in my head.
In SelfAlwaysShown i would expect just myself.
What should i expect in a bin that includes participants that match: SelfNotAlwaysShown

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about calling the bin enum sth like BinType since it is not the bin itself?

Copy link
Member Author

@robintown robintown Jul 17, 2024

Choose a reason for hiding this comment

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

I would say that a SortingBin is the bin itself, on a conceptual level. In my mind, calling it BinType would imply that you can instantiate multiple distinct Bins for any BinType, but a Bin doesn't really need to encode any more information than BinType does, so… a SortingBin already seems like a unique reference to the area that we see on screen.

Does that make sense at all? What is a 'bin', to you?

src/state/CallViewModel.ts Outdated Show resolved Hide resolved
@robintown
Copy link
Member Author

The reason I've split up local and remote user media into different types is that they have distinct sets of properties (local media can be always shown/never shown, remote media has a volume, etc.), and I wanted to enforce that the views that consume them would be fully aware of their differences in functionality. It's the same reason why screen share media has its own type.

@robintown
Copy link
Member Author

Since you've approved this I'm going to merge it and continue working through the PR stack - I'm happy to iterate later on the SortingBin topic if you do have concerns there!

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

Successfully merging this pull request may close these issues.

2 participants