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

UserBubble is empty in v4.1.0 #2200

Closed
julien-nc opened this issue Sep 2, 2021 · 11 comments · Fixed by #2236
Closed

UserBubble is empty in v4.1.0 #2200

julien-nc opened this issue Sep 2, 2021 · 11 comments · Fixed by #2236
Assignees
Labels
bug Something isn't working component Component discussion and/or suggestion feature: userbubble Related to the userbubble component regression Regression of a previous working feature

Comments

@julien-nc
Copy link
Contributor

UserBubble is now rendered as an empty div (the wrapper):

<div data-v-5c0e21af="" data-v-ae66bf1a="" trigger="hover focus" class="user-bubble__wrapper"> </div>

which means the Popover trigger slot is not rendered anymore.

@raimund-schluessler This bug was introduced in #2160 (8228b6e). I tried to revert src/components/UserBubble/UserBubble.vue like it was before this commit and it works fine.

I didn't investigate further what's problematic in this commit. Maybe this syntax change is not strictly equivalent as before and some attributes are not visible inside the <template> tag.

@julien-nc julien-nc added bug Something isn't working component Component discussion and/or suggestion feature: userbubble Related to the userbubble component labels Sep 2, 2021
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Sep 2, 2021

Hm, it seems that this only happens when the default slot is empty. I.e. this renders something:

<UserBubble
	:margin="4"
	:size="30"
	display-name="Administrator"
	user="admin">
	Test stuff text
	<template #title>
		<a href="#"
			title="Remove user"
			class="icon-close"
			@click="alert" />
	</template>
</UserBubble>

whereas this does not:

<UserBubble
	:margin="4"
	:size="30"
	display-name="Administrator"
	user="admin">
	<template #title>
		<a href="#"
			title="Remove user"
			class="icon-close"
			@click="alert" />
	</template>
</UserBubble>

@eneiluj Can you confirm this?

@julien-nc
Copy link
Contributor Author

@raimund-schluessler yes I confirm. I tested even without the #title template. This renders well:

<UserBubble
	:user="approver.type === 'user' ? approver.entityId : undefined"
	:display-name="approver.displayName"
	:avatar-image="avatarImageClass"
	:size="24">
	blabla
</UserBubble>

and this does not:

<UserBubble
	:user="approver.type === 'user' ? approver.entityId : undefined"
	:display-name="approver.displayName"
	:avatar-image="avatarImageClass"
	:size="24" />

@raimund-schluessler
Copy link
Contributor

What did the UserBuble component render before the changes in 8228b6e when you let the default slot empty? It might be an issue with the VPopover component which was kind of hidden by the different syntax used before.

@julien-nc
Copy link
Contributor Author

You mean what did it render exactly? I can paste a full dom element if you want.
But approximately it rendered the bubble without any popover 😁

@raimund-schluessler
Copy link
Contributor

Well, the bubble is the popover (or more specifically, the trigger of the popover). That's maybe the issue here.

@nickvergessen
Copy link
Contributor

This also breaks the notifications with the nextcloud-vue version 4.1 or later.

@nickvergessen
Copy link
Contributor

nickvergessen commented Sep 13, 2021

DOM with 4.0.3

<div data-v-6da16f10="" data-v-30428f72="" class="mention rich-text--component" attrs="[object Object]" key="h-2">
	<div data-v-28b47c60="" data-v-6da16f10="" trigger="hover focus" class="user-bubble__wrapper">
		<div data-v-28b47c60="" slot="trigger" class="user-bubble__content" style="height: 20px; line-height: 20px; border-radius: 10px;">
			<div data-v-e1bb6d38="" data-v-28b47c60="" tabindex="-1" aria-label="Avatar of Bennet Bernetto" role="" class="avatardiv popovermenu-wrapper user-bubble__avatar" style="--size: 16px; line-height: 16px; font-size: 9px; margin-left: 2px;" margin="2">
				<img data-v-e1bb6d38="" src="/index.php/avatar/d4a24b89-fb28-44c8-be16-7bdce9cd02af/16" srcset="/index.php/avatar/d4a24b89-fb28-44c8-be16-7bdce9cd02af/16 1x, /index.php/avatar/d4a24b89-fb28-44c8-be16-7bdce9cd02af/32 2x, /index.php/avatar/d4a24b89-fb28-44c8-be16-7bdce9cd02af/64 4x" alt=""> 
				<!----> 
				<!----> 
				<!---->
			</div> 
			<span data-v-28b47c60="" class="user-bubble__title">
				Bennet Bernetto
			</span>
			<!---->
		</div>
	</div>
</div>

DOM with 4.1.0:

<div data-v-6da16f10="" data-v-30428f72="" class="mention rich-text--component" attrs="[object Object]" key="h-2">
	<div data-v-1602ea3f="" data-v-6da16f10="" trigger="hover focus" class="user-bubble__wrapper"> </div>
</div>

@julien-nc
Copy link
Contributor Author

@raimund-schluessler We could revert changes made to src/components/UserBubble/UserBubble.vue in #2160 (8228b6e) as a temporary fix. Then you would have plenty of time to think about how to tackle this new slot syntax issue 😁.

@nickvergessen
Copy link
Contributor

It reminds me of nextcloud/spreed#2435

Where we moved to the new syntax in nextcloud/spreed#2424 but had to revert some, as they didn't work with the new syntax for whatever reason. I think it's the same problem here.

@raimund-schluessler
Copy link
Contributor

@raimund-schluessler We could revert changes made to src/components/UserBubble/UserBubble.vue in #2160 (8228b6e) as a temporary fix. Then you would have plenty of time to think about how to tackle this new slot syntax issue 😁.

Fine with me.

@raimund-schluessler raimund-schluessler self-assigned this Sep 13, 2021
julien-nc pushed a commit that referenced this issue Sep 13, 2021
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@raimund-schluessler raimund-schluessler added the regression Regression of a previous working feature label Sep 13, 2021
@raimund-schluessler
Copy link
Contributor

I now understand what the problem is. If the default slot is empty, we don't use a Popover component, but a simple div:
https://github.com/nextcloud/nextcloud-vue/blob/16cf329d4c8d8b4ad99fcc4b811c624f121ce0e4/src/components/UserBubble/UserBubble.vue#L203-L207

A simple div obviously has no #trigger slot and since it is wrapped in a template now, it simply does not show at all. Before we had a div directly in the markup, which would also render in a div.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component Component discussion and/or suggestion feature: userbubble Related to the userbubble component regression Regression of a previous working feature
Projects
None yet
3 participants