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

In fullscreen, neither the call actions menu nor the personal controls menu works, the viewer neither #5027

Closed
marcoambrosini opened this issue Jan 26, 2021 · 14 comments · Fixed by #5488

Comments

@marcoambrosini
Copy link
Member

In fullscreen, neither the call actions menu nor the personal controls menu works, the viewer neither.

@PVince81
Copy link
Member

probably a containment issue:
image

the popover element is appended into the body but the element we picked for fullscreen is not in that hierarchy

@PVince81 PVince81 added this to the 💔 Backlog milestone Feb 26, 2021
@PVince81
Copy link
Member

also broken in Talk 10.0.5

@nickvergessen
Copy link
Member

Also broken in 9
Basically since the menus use the popover and that was moved to the body instead of being in place.
But i think in reset versions the popover menu has an option where to mount it:

https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/Actions/Actions.vue#L297

If we set that to the item we fullscreen it should work again?

@PVince81
Copy link
Member

yes, that or boundariesElement.

and find a good way to set the main container globally to all vues, like the store or mixins.

@nickvergessen
Copy link
Member

yes, that or boundariesElement.

Well additionally maybe, but it needs to "container"ed in the fullscreened element because everything else is not rendered/visible.

and find a good way to set the main container globally to all vues, like the store or mixins.

Please don't break everything again 🙈

@PVince81
Copy link
Member

I didn't mean reparenting, only exposing a reusable value to avoid repeating "document.getElementById('content-vue')" everywhere. I noticed that "this.$root.$el" contains that element too but the first time it's evaluated it contains the old element "#content" which gets replaced after Vue finishes it initialization. So not usable in a computed property.

I guess a helper method will do, then, so that this gets reevaluated every time.

@PVince81 PVince81 self-assigned this Feb 26, 2021
@PVince81
Copy link
Member

PVince81 commented Feb 26, 2021

@PVince81
Copy link
Member

PR for the Actions bit: #5270

others will need more work as they require to extend the nextcloud-vue components...

@PVince81
Copy link
Member

PVince81 commented Mar 2, 2021

  • check if some tooltips also need a container: it might be possible to update their defaultContainer value when switching to and from fullscreen

@PVince81
Copy link
Member

PVince81 commented Mar 2, 2021

PR for nextcloud-vue to add the container prop: nextcloud-libraries/nextcloud-vue#1734

@PVince81
Copy link
Member

PVince81 commented Mar 2, 2021

PR for talk app adjustments: #5291

@PVince81
Copy link
Member

PVince81 commented Mar 2, 2021

the tooltips are not working either.

adding this Tooltip.options.defaultContainer = '#content-vue' in main.js makes it work globally but could potentially affect tooltips in the header.

I've also tried to set that value after switching to fullscreen, but it requires a re-render of the matching components to make it work

@PVince81
Copy link
Member

PVince81 commented Mar 4, 2021

all merged, now only the tooltip issue remains

@PVince81
Copy link
Member

PVince81 commented Apr 15, 2021

adding this Tooltip.options.defaultContainer = '#content-vue' in main.js makes it work globally but could potentially affect tooltips in the header.

I've verified that the tooltip default only affects the Talk app by logging that setting inside another app that uses the library.
More details in my PR: #5488

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants