-
Notifications
You must be signed in to change notification settings - Fork 924
feat(useOverlay): add on method to listen emits from component
#5244
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
base: v4
Are you sure you want to change the base?
Conversation
on method to listen emits from component
commit: |
|
I forget the API instance documentation + Caveats section miss a warning about the limitation of the composable. We need to discuss about its limitation is caused by typescript limitations |
| const overlay = getOverlay(id) | ||
|
|
||
| if (!overlay.emits) overlay.emits = {} | ||
| overlay.emits[event as string] = callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| overlay.emits[event as string] = callback | |
| // For close events, preserve the default close behavior by chaining callbacks | |
| if (event === 'close') { | |
| const existingHandler = overlay.emits[event as string] | |
| if (existingHandler) { | |
| overlay.emits[event as string] = (...args: Parameters<typeof callback>) => { | |
| callback(...args) | |
| existingHandler(...args) | |
| } | |
| } else { | |
| overlay.emits[event as string] = callback | |
| } | |
| } else { | |
| overlay.emits[event as string] = callback | |
| } |
The on() method replaces the default close event handler instead of preserving it, which breaks automatic overlay closure when registering close event listeners.
View Details
Analysis
useOverlay.on() replaces default close handler breaking automatic overlay closure
What fails: useOverlay.on('close', callback) in src/runtime/composables/useOverlay.ts:214 replaces the default close handler instead of chaining it, preventing automatic overlay closure when user callbacks don't explicitly call modal.close()
How to reproduce:
const { create } = useOverlay()
const modal = create(Component)
modal.open()
// Register close handler that doesn't call modal.close()
modal.on('close', (value) => {
console.log('Close event received:', value)
// No modal.close() call here
})
// When component emits close event, overlay stays open
component.$emit('close', 'result') // Overlay remains openResult: Overlay stays open because default (value) => close(id, value) handler is replaced by user callback
Expected: Overlay should close automatically even when user callback doesn't call modal.close(), preserving the default close behavior established in create() at line 118
Root cause: Line 214 uses direct assignment overlay.emits[event as string] = callback instead of chaining handlers for close events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@genu what about this constructive feedback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I see the issue -- this is valid.
The close event is a special kind of event that also should actually hide the modal.
In the original design (and briefly explained in the docs), the close event is used to call close(id, value).
This is the key issue with the close event:
There are two ways to close an overlay:
1. The component emits a close event.
2. The user clicks the X button which hides the modal
In both cases, we need to:
- Handle the event using the .on(...) callback (Missing this piece for option 2)
- Handle the event using the resolvedPromise
In the original design, we relied on the resolvePromise to handle both cases.
In this new event handling with the .on(...) system, we are treating the close event like any other component event. For close specifically, we need to also account for the second way of close an overlay.
Let me know if you need more clarity on this. I'm sure there is room to refactor some things to make it cleaner in this area, this might be a good opportunity for it.
For example,
- Should we implement an internal
closeevent handler and fire and handle it automatically in both scenarios in theonAfterLeave?. Then, treat any.on('close', ...)andconst res = await modal.open(...)as a user event that gets called automatically. - Should we expand our built-in events for more granularity, like "close", "beforeClose", "afterClose". We may not do this now, but we can think about this if we do some refactoring.
Let me know what you think.
I can spend some more time this week to give you further feedback or help out with this PR.
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
60e4da6 to
dc8040f
Compare
|
@genu can you take a look? |
|
Thanks for this @edimitchel I think it looks good. The I think it may be even a good idea to deprecate the old way of handling events. Having multiple ways could be confusing. What do you think about that? We could maybe, add a const modal = overlay.create(LazyModalExample, {
props: {
count: count.value,
onClose: (value: number) => {
console.log('Modal closed with value:', value)
}
}
})Otherwise, we could depreciate them in a future version as a breaking change |
π Linked issue
Resolved #5233
β Type of change
π Description
Introducing this new
onmethod permits to easy bind emits event before opening for let developer to only think about props in create/open/patch methods.π Checklist