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

Popover component event handling #2961

Open
MrVauxs opened this issue Nov 13, 2024 · 5 comments
Open

Popover component event handling #2961

MrVauxs opened this issue Nov 13, 2024 · 5 comments
Assignees
Labels
feature request Request a feature or introduce and update to the project.
Milestone

Comments

@MrVauxs
Copy link

MrVauxs commented Nov 13, 2024

Describe the feature in detail (code, mocks, or screenshots encouraged)

Zag's popup elements are based on a button element which causes problems when you want to add onclick events and such, or when you want to wrap an existing button with a Tooltip.

It would be great to be able to pass event handlers to Zag's button since it cannot be removed to allow users to decide the markup themselves.

What type of pull request would this be?

Enhancement

Provide relevant links or additional information.

#skeleton-next talk in Discord: https://discord.com/channels/1003691521280856084/1191790107867488316/1306346031528673290

they all rely on onOpenChange, so I think we'll need this change for all four (popover, tooltip, modal, combobox)

@MrVauxs MrVauxs added the feature request Request a feature or introduce and update to the project. label Nov 13, 2024
@endigo9740 endigo9740 added this to the v3.0 (Next) milestone Nov 13, 2024
@endigo9740
Copy link
Contributor

endigo9740 commented Dec 12, 2024

@Hugos68 I know you've got limited availability right now, no worries if you don't get back to this until later, but either way I could use your help. Currently in the Tooltip component for example we have this:

<button {...api.getTriggerProps()} class="{triggerBase} {triggerBackground} {triggerClasses}">
   {@render trigger?.()}
</button>

So Zag is spreading it's own attributes to the button as one would expect. And if I log the output of api.getTriggerProps() I get the following:

Screenshot 2024-12-12 at 3 19 37 PM

So there is already an onclick in place. But I can't seem to find any sort of reference in Zag to how we hook into this or what it's used for. I'd be leery of us implementing our own onclick handler on top of this, as I would expect that would be considered a duplicate and cause issues.

As far as the API Zag exposes, there doesn't appear to be anything related to the button click event. They have onOpenChange() which triggers when the popover is shown/hidden on hover, but that's not quite what's being requested here.

Just thought I'd reach out to you before I ask Zag about this. Any ideas are welcome.

@Hugos68
Copy link
Contributor

Hugos68 commented Dec 13, 2024

Zag exposes a mergeProps function to basically merge listeners from users and zag and still have the component function, see: https://zagjs.com/overview/composition#event-composition

We can definitely support this if we want to 👍

@endigo9740
Copy link
Contributor

@Hugos68 ok that actually looks fairly straight forward. I'll look to get that setup today. Thank you!

@endigo9740
Copy link
Contributor

endigo9740 commented Dec 13, 2024

@phamduylong this would be a great candidate if you're looking for something to work on! We'll just need to implement an onclick event property for all four Svelte popover components using the merge method Hugo linked above.

@phamduylong
Copy link
Contributor

On it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request a feature or introduce and update to the project.
Projects
None yet
Development

No branches or pull requests

4 participants