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

New one-on-one layout #2417

Merged
merged 6 commits into from
Jul 18, 2024

Conversation

robintown
Copy link
Member

@robintown robintown commented Jun 7, 2024

Based on #2416

Closes #1244

@robintown
Copy link
Member Author

TODO: If you switch to spotlight while connecting, the app crashes

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
Because we were hiding even the local participant during initial connection, there would be no participants, and therefore nothing to put in the spotlight. The designs don't really tell us what the connecting state should look like, so I've taken the liberty of restoring it to its former glory of showing the local participant immediately.
@robintown robintown force-pushed the one-on-one-layout branch from 7ec04fe to a16f235 Compare July 12, 2024 19:52
@robintown robintown mentioned this pull request Jul 17, 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.
I might have missed it but as far as i can tell this does never render tiles in portrait mode.
I think in 1:1 that is the one situation where this is totally doable.
I think right now it behaves the other way around which is already great (landscape will be rendered as landscape on a portrait screen).
But portrait media being rendered as portrait on a landscape screen would also be nice. (maybe with a little bit of a cutoff to not make it too small 9:16 becomeing 2:3 or similar)

Comment on lines +119 to +128
>
<Slot
className={classNames(styles.slot, styles.local)}
id={localTileModel.vm.id}
model={localTileModel}
onDrag={onDragLocalTile}
data-block-alignment={pipAlignmentValue.block}
data-inline-alignment={pipAlignmentValue.inline}
/>
</Slot>
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. The slot for the local media feed is a child of the remote participant slot?
That makes actually.
So the local tile is always inside the parent/remote tile. Will this also hold true if we have a portrait remote media feed on a landscape screen. in that case i can imagine we would like the local tile to be outside the remote tile?
(currently we use the "show grey borders" (fit to frame) approach but in theory it would be nice to have the tile be the correct aspect ratio already)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, making it be a child was the easiest way to get the right positioning. As I've commented below I don't know how to account for landscape media, currently (but neither does the old one-on-one layout).

@robintown robintown changed the base branch from livekit to new-call-layouts July 18, 2024 12:52
@robintown robintown removed the X-Blocked Cannot be merged due to external dependencies label Jul 18, 2024

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
@robintown
Copy link
Member Author

Thanks for the suggestions, I'm pretty happy with how the 2:3 aspect ratio looks on mobile now:

Detecting when the media we're receiving should be landscape is something I don't know how to solve, currently :/ I think that'll have to be outside the scope of this PR. So the best I can do for now is assume that media should be portrait whenever the window is sufficiently narrow, which is the same as how the old one-on-one layout behaves.

@robintown robintown merged commit bcc06d8 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repace all remaining usages of VideoGrid with NewVideoGrid
2 participants