Use the popover component for the avatar menu #831

marcoambrosini · 2020-01-22T14:19:11Z

Signed-off-by: Marco Ambrosini marcoambrosini@pm.me

fix nextcloud/server#18634

package.json

src/components/Avatar/Avatar.vue

skjnldsv · 2020-08-21T10:31:12Z

Ping @ma12-co :)

MorrisJobke · 2020-09-16T07:54:15Z

@ma12-co 🏓

marcoambrosini · 2020-09-16T15:02:31Z

Pong!

skjnldsv · 2020-10-01T20:37:27Z

src/components/Avatar/Avatar.vue

 import { generateUrl } from '@nextcloud/router'
 import Tooltip from '../../directives/Tooltip'
 import usernameToColor from '../../functions/usernameToColor'
 import { userStatus } from '../../mixins'
+import Popover from '../Popover/Popover'
+import PopoverMenuItem from '../PopoverMenu/PopoverMenuItem'


Not sure we want to keep this PopoverMenuItem component. What about Actions?

Ideally yes! Followup?
This should actually fix many placement bugs for now

skjnldsv · 2020-10-12T12:02:51Z

@ma12-co rebase?

PVince81 · 2021-01-18T16:53:19Z

Okay, I found the culprit: when clicking, first the popover will open with an empty contents because at this time the contacts menu is not loaded yet. Only then once loaded, it will render into the contents which triggers a resize.

Next up: to find a way to not display the popover while loading.

PVince81 · 2021-01-18T17:06:45Z

It's worse: even if I numb "toggleMenu", it seems the click on the trigger will still trigger the v-tooltip, even though "VPopover" has "open=false". So it seems more work is needed here to get all this under control and avoid redundancy.

VPopover is always visible even when "open" is false. This fix makes it hidden visually off screen instead. Solves issues with flying arrows when opening and closing some action menus. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81 · 2021-01-18T17:37:37Z

Okay, so I managed to fix the flying arrow issue from #1610 by making the popover invisible until "open" is actually true.

While this works for the call view popover like in #1610, it doesn't help with the avatar menu

The problem with the avatar menu it seems to be that whenever the menu is loaded and "open" is set to true, the v-tooltip will already render the contents, but in the wrong location. And this triggers the resize handler which will put the tooltip at the correct position.

So next would be to try to find a way to pre-render the tooltip offscreen while opening the first time, and then letter popper reposition it...

The interesting thing now is that opening the menu will now always show it shifted first. Even when reopening. Before these changes it would only do so on the first open.

Needs further research... 🤯

marcoambrosini · 2021-01-19T08:24:43Z

Thanks for taking over @PVince81 :)

PVince81 · 2021-01-19T09:45:14Z

I've tried to add a loading spinner "fake menu entry" to make sure we got some initial contents in the tooltip. This approach makes it possible for the tooltip to be directly positioned in the right place. Then when the single menu entry loads it doesn't move.
So far so good.
However, as soon as there are multiple menu entries, the popover will jump again after loading.
Furthermore, even with a single one, the quick switch between loading spinner is annoying (that menu loads quickly usually).

Yet another approach I found (without loading spinner) was to add an additional delay before actually showing the tooltip, to let the libraries time to recompute its position before it actually becomes visible. Basically a delay between setting :open to true and removing an extra visibility: hidden from the tooltip. I guess I'll go with the latter now as I feel we've lost enough time with this mess.

Whenever the popover is set to "open" first, if the contents hasn't already been populated in the current tick, the popover's position will appear in the wrong location. Then, directly in the next tick its position is corrected based on the newly appeared content. To avoid the visual glitch we first keep it with "visibility: hidden" upon opening, and then display it once its position has been calculated. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81 · 2021-01-19T10:15:02Z

Ok, the fix is in with "visibility: hidden" with a delay.

I've also added a loading spinner on the trigger, which lets the user know that something is loading so that the UI doesn't feel sluggish while loading the menu.

Now while the avatar menus work fine now, there's still a problem with the popover from the call view: when you click the actions menu at the bottom, the popover does appear and causes a scrollbar glitch. It seems the "__hidden" class is not enough and needs to be applied on the popover element itself instead.

When initially hiding the popover during size / position calculation, we need to override the popover's initial visibility and position to avoid glitches like scrollbars appearing. Once the correct position has been calculated by v-tooltip/popper, we can remove the strong hiding class. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81 · 2021-01-19T10:31:59Z

Okay, all works now...

Tested with Talk:

avatar menu in messages list: spinner appears, menu appears directly in correct position
avatar menu in participants list: spinner appears, menu appears directly in correct position
popover top menu in callview appears with no arrow animation glitch
popover bottom menu in callview appears with no arrow animation glitch, and no scrollbar glitch (scrollbar was appearing for a millisecond then disappearing)

@ma12-co can you test and review ?

PVince81 · 2021-01-19T10:33:38Z

I know, the hack is ugly but that's the only way I found to work around the fact that the library will display the tooltip first before repositioning, instead of showing it off screen, evaluating the size, and then repositioning. Maybe there are better ways that would require patching the libs. I saw there will be new versions of the libs (v-tooltip and popper.js), something to look into separately.

PVince81 · 2021-01-19T11:50:17Z

Now a question is still in the air: why use the Popover directly and not use Actions instead ? The latter already has keyboard navigation and everything needed to make it accessible 🤔

PVince81 · 2021-01-19T13:40:30Z

Now a question is still in the air: why use the Popover directly and not use Actions instead ? The latter already has keyboard navigation and everything needed to make it accessible thinking

I had a quick try with Actions and it comes with additional challenges, and the hack for preventing the repositioning glitch doesn't work there: #1670

Code has changed massively

marcoambrosini

Tested and works, cannot approve as I'm the original author 😅

ChristophWurst

Minor remarks, looks good otherwise 👍

src/components/Avatar/Avatar.vue

src/components/Popover/Popover.vue

PVince81 · 2021-01-20T08:41:23Z

Crap, I retested this and I still see the position shifting glitch from time to time... Might be a race against v-tooltip's timers...
Anyway, at least the latest hacks reduce the probability.

Eventually we'll need to look into upgrading v-tooltip/popper.js, maybe patching it to make it work properly...

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81

Approving on behalf of @ma12-co

PVince81 · 2021-01-26T08:58:23Z

regression: #1681

skjnldsv reviewed Jan 22, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

skjnldsv reviewed Jan 22, 2020

View reviewed changes

src/components/Avatar/Avatar.vue Show resolved Hide resolved

skjnldsv reviewed Jan 22, 2020

View reviewed changes

src/components/Avatar/Avatar.vue Outdated Show resolved Hide resolved

juliusknorr reviewed Jan 22, 2020

View reviewed changes

src/components/Avatar/Avatar.vue Outdated Show resolved Hide resolved

marcoambrosini force-pushed the feature/noid/use-the-popover-component-for-the-avatar-menu branch from 9c42090 to b79d7c6 Compare January 23, 2020 07:20

This comment has been minimized.

Sign in to view

georgehrke reviewed Aug 20, 2020

View reviewed changes

src/components/Avatar/Avatar.vue Outdated Show resolved Hide resolved

skjnldsv mentioned this pull request Aug 21, 2020

files sidebar sharee/share owner action menu cut off nextcloud/server#18634

Closed

skjnldsv added 2. developing Work in progress enhancement New feature or request feature: avatar Related to the avatar component labels Aug 21, 2020

marcoambrosini force-pushed the feature/noid/use-the-popover-component-for-the-avatar-menu branch 3 times, most recently from e37a861 to 70520ea Compare September 16, 2020 14:49

marcoambrosini marked this pull request as ready for review September 16, 2020 15:01

marcoambrosini requested review from juliusknorr and skjnldsv September 16, 2020 15:01

marcoambrosini added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 16, 2020

marcoambrosini requested a review from MorrisJobke September 16, 2020 15:02

skjnldsv requested a review from ChristophWurst October 1, 2020 20:35

skjnldsv reviewed Oct 1, 2020

View reviewed changes

skjnldsv previously approved these changes Oct 2, 2020

View reviewed changes

skjnldsv requested a review from georgehrke October 19, 2020 12:07

skjnldsv removed the 3. to review Waiting for reviews label Oct 19, 2020

PVince81 added 2 commits January 18, 2021 18:33

Fix arrow animation of popover

b0ab67e

VPopover is always visible even when "open" is false. This fix makes it hidden visually off screen instead. Solves issues with flying arrows when opening and closing some action menus. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Adjust avatar menu display conditions

1135ba0

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81 mentioned this pull request Jan 19, 2021

Feature/noid/avatar menu use actions component #1670

Closed

5 tasks

PVince81 requested a review from skjnldsv January 19, 2021 13:51

marcoambrosini commented Jan 19, 2021

View reviewed changes

ChristophWurst approved these changes Jan 19, 2021

View reviewed changes

src/components/Avatar/Avatar.vue Outdated Show resolved Hide resolved

src/components/Popover/Popover.vue Show resolved Hide resolved

Adjustments after review

1aed77d

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81 approved these changes Jan 20, 2021

View reviewed changes

PVince81 merged commit b5e63ce into master Jan 20, 2021

PVince81 deleted the feature/noid/use-the-popover-component-for-the-avatar-menu branch January 20, 2021 08:47

This was referenced Jan 26, 2021

[3.5.2] Popover - Trigger doesn't trigger anymore #1681

Closed

Actions menu / Popover flying arrow on close #1610

Closed

Avatar popover glitch when opened the first time #1683

Closed

Revert workaround for popper glitch, fix regression #1685

Merged

gary-kim mentioned this pull request May 27, 2021

Dashboard Recent Bug nextcloud/server#27106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the popover component for the avatar menu #831

Use the popover component for the avatar menu #831

marcoambrosini commented Jan 22, 2020 •

edited by skjnldsv

Loading

This comment has been minimized.

This comment has been minimized.

skjnldsv commented Aug 21, 2020

MorrisJobke commented Sep 16, 2020

marcoambrosini commented Sep 16, 2020

skjnldsv Oct 1, 2020

marcoambrosini Oct 2, 2020

skjnldsv commented Oct 12, 2020

PVince81 commented Jan 18, 2021

PVince81 commented Jan 18, 2021

PVince81 commented Jan 18, 2021

marcoambrosini commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

marcoambrosini left a comment

ChristophWurst left a comment

PVince81 commented Jan 20, 2021

PVince81 left a comment

PVince81 commented Jan 26, 2021

Use the popover component for the avatar menu #831

Use the popover component for the avatar menu #831

Conversation

marcoambrosini commented Jan 22, 2020 • edited by skjnldsv Loading

This comment has been minimized.

This comment has been minimized.

skjnldsv commented Aug 21, 2020

MorrisJobke commented Sep 16, 2020

marcoambrosini commented Sep 16, 2020

skjnldsv Oct 1, 2020

Choose a reason for hiding this comment

marcoambrosini Oct 2, 2020

Choose a reason for hiding this comment

skjnldsv commented Oct 12, 2020

PVince81 commented Jan 18, 2021

PVince81 commented Jan 18, 2021

PVince81 commented Jan 18, 2021

marcoambrosini commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

PVince81 commented Jan 19, 2021

marcoambrosini left a comment

Choose a reason for hiding this comment

ChristophWurst left a comment

Choose a reason for hiding this comment

PVince81 commented Jan 20, 2021

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Jan 26, 2021

marcoambrosini commented Jan 22, 2020 •

edited by skjnldsv

Loading