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 ability to collapse the grid view #4363

Merged
merged 8 commits into from
Oct 19, 2020

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Oct 13, 2020

To be able to focus on the video / screen of the current presenter.

Fixes #4245

Todos

@PVince81
Copy link
Member Author

After talking with @ma12-co we decided to keep my PR and I've taken a similar approach from #4365 and added (and fixed) the slide animation.

Next up is extracting the local call controls to a higher layer to make them always visible and available even when collapsed.

@PVince81
Copy link
Member Author

PVince81 commented Oct 14, 2020

  • sort out isBig attribute and expected logic
  • wire up other missing attributes
    • isSidebar

@PVince81
Copy link
Member Author

current state when open:
image

and collapsed:
image

@PVince81 PVince81 force-pushed the feature/4245/ui-hide-bottom-videos branch from 459535b to d257ed8 Compare October 14, 2020 13:37
@PVince81
Copy link
Member Author

PVince81 commented Oct 14, 2020

most stuff sorted out, but one thing remaining:

  • network quality warning broken when grid is collapsed: quickest would be to move it back to the media controls location as it creates an extra temporary icon.

I do wonder if there is another kind of interaction we could use to show that warning and not clutter that bottom right corner with a bubble.

@PVince81
Copy link
Member Author

next challenge:
image

@PVince81
Copy link
Member Author

ok, it will look like this:
image

@PVince81 PVince81 force-pushed the feature/4245/ui-hide-bottom-videos branch from d257ed8 to f10fbae Compare October 14, 2020 14:34
@PVince81 PVince81 marked this pull request as ready for review October 14, 2020 15:02
@PVince81
Copy link
Member Author

Please review.

I'm optimistic that it will work in sidebar mode correctly, but still want to confirm. I can't test that bit because of a server issue nextcloud/server#23445

@PVince81
Copy link
Member Author

PVince81 commented Oct 15, 2020

  • BUG: missing mute and video buttons in sidebar mode

(tested using this PR merged with #4291)

@PVince81
Copy link
Member Author

I've fixed the sidebar mode. It required me to move the LocalMediaControls from the Grid to the CallView.

I've also tested the "network connection warning" mode and it looks fine in sidebar mode.

@PVince81
Copy link
Member Author

image

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

  • shouldn't this transition be applied to the .grid-main-wrapper div?
    At the moment the transition feels a bit weird, it seems like .grid-main-wrapper comes in without transition and its content slides up in it

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

I get this error when switching to grid:
Screenshot from 2020-10-19 13-05-18

@PVince81
Copy link
Member Author

when testing I had the latest nextcloud-vue master included.
retrying with the latest spreed master

@PVince81
Copy link
Member Author

PVince81 commented Oct 19, 2020

Got it, the exact steps are:

  1. Start a call with three people
  2. Collapse the strip
  3. Switch to grid view
  4. Switch back to speaker view

The reason is that isStripe() watcher is re-generating the grid in makeGrid, and the latter function is trying to access the grid object while it's not visible. I'll try adding a check there to avoid calling makeGrid when collapsed.

Too many states :-S

@PVince81 PVince81 force-pushed the feature/4245/ui-hide-bottom-videos branch from 572f940 to 341a206 Compare October 19, 2020 13:01
@PVince81
Copy link
Member Author

@ma12-co I've fixed the issue when switching back and forth to grid while collapsed

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Amazing :)
One last cool thing would be a tooltip that shows up on hover that says something like "Hide videos" and "Show videos"

PVince81 and others added 8 commits October 19, 2020 17:18
To be able to focus on the video / screen of the current presenter.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Co-authored-by: Marco Ambrosini <marcoambrosini@pm.me>
Moved LocalMediaControls to the Grid view to make sure it's always
visible even when the grid is collapsed.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Moved LocalMediaControls to the CallView to make sure they are available
in any mode, including the sidebar one.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Add 100% height only when in grid view.
Added LocalMediaControls to LocalVideo for when in grid view, properly
centered.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
In order to keep the collapse logic and styles in a single location, the
one on one LocalVideo was removed and now the Grid view's stripe is
reused with a transparent background.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Make sure the top component fills the full height while running the
animation, to avoid showing some black background there.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
When the stripe (internally a grid) is collapsed, it should not attempt
to rebuild itself.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Member Author

it's not videos if you only see avatars there, so not sure about the text.
maybe we can look into tooltips separately as I noticed there are more tooltips missing in the call view

@PVince81 PVince81 force-pushed the feature/4245/ui-hide-bottom-videos branch from 341a206 to b047b63 Compare October 19, 2020 15:19
@PVince81 PVince81 merged commit 7334213 into master Oct 19, 2020
@PVince81 PVince81 deleted the feature/4245/ui-hide-bottom-videos branch October 19, 2020 16:24
@danxuliu
Copy link
Member

Since this pull request when the call view is shown in the sidebar the media controls are a bit off from the video (I have tested with nextcloud/server@fd178b9^ and also rebasing #4291 and using it against nextcloud/server@766590d204^, so it should not be a problem of an outdated style from server):

Before:
Files-Sidebar-Controls-Before

After:
Files-Sidebar-Controls-After

Is that intended, or known?

@PVince81
Copy link
Member Author

I didn't observe this before. Can you make a ticket and provide steps to reproduce and maybe also window size ?
With all the different tests I made I didn't see that.

The reason is that now the controls are not part of the video box any more, they are outside. But how did you manage to have the video box smaller than usual ?

@marcoambrosini
Copy link
Member

We really need to implement tests for the call view. There are so many scenarios that I's becoming impossible to test all of them whenever someone changes something there

@PVince81
Copy link
Member Author

I've raised #4440 for adding automated tests for the frontend

@PVince81
Copy link
Member Author

@ma12-co while we might be able to test the code and logic, we can't test layout and styling stuff, so if a background color looks off or a container is not stretched correctly, we'd still need some manual testing to find that out. However we wouldn't need to retest all the logic.

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.

Add option to hide video thumbnails of users while someone shares the screen
4 participants