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

fix(NcPopover): check trigger a11y compatible with Vue 3 #5110

Merged
merged 1 commit into from Jan 22, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jan 22, 2024

☑️ Resolves

Popover content is usually only the trigger. So no need to check content in a slot component.

Trigger's parent DOM node is accessible by $refs of Floating-Vue.

For Vue 3 only requires to change refs from Floating-Vue.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added 3. to review Waiting for reviews feature: popover Related to the popovermenu component labels Jan 22, 2024
@ShGKme ShGKme self-assigned this Jan 22, 2024
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@raimund-schluessler
Copy link
Contributor

Shouldn't this trigger the warning? But it does not in the docs (unless they aren't built as debug, but I think they are). It also gives no warning for master.

<template>
	<div style="display: flex">
		<NcPopover>
			<!-- Take "attrs" from the slot props -->
			<template #trigger>
				<!-- Bind attrs as the button attrs -->
				<button>
					I am a custom button in the trigger2
				</button>
			</template>

			Hi! 🚀
		</NcPopover>
	</div>
</template>

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 22, 2024

Works for me...

image

@raimund-schluessler
Copy link
Contributor

Shouldn't this trigger the warning? But it does not in the docs (unless they aren't built as debug, but I think they are). It also gives no warning for master.

<template>
	<div style="display: flex">
		<NcPopover>
			<!-- Take "attrs" from the slot props -->
			<template #trigger>
				<!-- Bind attrs as the button attrs -->
				<button>
					I am a custom button in the trigger2
				</button>
			</template>

			Hi! 🚀
		</NcPopover>
	</div>
</template>

On an unrelated note, this example triggers a focus-trap warning: 🙈
Your focus-trap must have at least one container with at least one tabbable node in it at all times.

@raimund-schluessler
Copy link
Contributor

Works for me...

Weird. I guess https://deploy-preview-5110--nextcloud-vue-components.netlify.app/#/Components/NcPopover is not debug mode then.

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 22, 2024

Weird. I guess https://deploy-preview-5110--nextcloud-vue-components.netlify.app/#/Components/NcPopover is not debug mode then.

It is... Let me check

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 22, 2024

Weird. I guess https://deploy-preview-5110--nextcloud-vue-components.netlify.app/#/Components/NcPopover is not debug mode then.

Oh, yes.

It is in OC.debug mode, but it uses production mode of Vue, so there is no Vue warnings.

Copy link
Contributor

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

Works locally 👍

@susnux susnux merged commit b570762 into master Jan 22, 2024
15 checks passed
@susnux susnux deleted the fix/nc-popover-a11y-check branch January 22, 2024 23:45
@Pytal Pytal mentioned this pull request Jan 23, 2024
@susnux susnux added the bug Something isn't working label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feature: popover Related to the popovermenu component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[next] Adjust warning in NcPopoverTriggerProvider
3 participants