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

Video tile redesign/refactor #1989

Merged
merged 5 commits into from
Jan 8, 2024
Merged

Conversation

robintown
Copy link
Member

@robintown robintown commented Dec 7, 2023

Depends on element-hq/compound-web#110

Closes #1764
Fixes one half of #1839 (local video is now mirrored correctly in the call view, but not in the lobby)

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ec06c84) 25.02% compared to head (1865ccb) 25.02%.
Report is 4 commits behind head on livekit.

Additional details and impacted files
@@           Coverage Diff            @@
##           livekit    #1989   +/-   ##
========================================
  Coverage    25.02%   25.02%           
========================================
  Files           48       47    -1     
  Lines         2394     2382   -12     
  Branches       438      434    -4     
========================================
- Hits           599      596    -3     
+ Misses        1744     1735    -9     
  Partials        51       51           
Flag Coverage Δ
unittests 25.02% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@robintown robintown marked this pull request as ready for review December 18, 2023 22:06
@robintown robintown requested a review from a team as a code owner December 18, 2023 22:06
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.

Thanks.
This is super satisfying to see how this also makes it a lot more readable (+ better to test)
Most of the code also was very striaght forward to review.

Playing around with it I found the following, is this intentional?

  • The user tile ("you") cannot be moved into the first position grid mode.

I will give this a approval since I am on holiday but before merging I think the "cannot be moved into the first position" should addressed (exept its by design. If so I would be curious why) and the TODO (move to vm in case that is easy enough for this PR)

src/room/InCallView.module.css Show resolved Hide resolved
src/video-grid/VideoTile.tsx Show resolved Hide resolved
@robintown
Copy link
Member Author

The user tile ("you") cannot be moved into the first position grid mode.

Ah right, I've seen this happening in the application for some months, so I don't believe it's a regression introduced here. The plan for fixing this is that, the new grid layouts won't allow for dragging and dropping anyways, so I'd like to move ahead with implementing them.

@robintown robintown merged commit 76c3125 into element-hq:livekit Jan 8, 2024
6 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.

'Profile' button doesn't work after switching tabs in settings
3 participants