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

Events emitted when modal is opened and closed #16

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ibanjo
Copy link

@ibanjo ibanjo commented Aug 2, 2019

Thank you for the wrapper, it made my day! I just added a couple (possibly) useful events, fired when the modal is opening/closing and after the show/hide methods have been called on the A11yDialog instance.

@morkro
Copy link
Owner

morkro commented Aug 3, 2019

Hey @ibanjo, thanks for your pull request! Just thinking, wouldn't these features potentially be better suited in user land? These events could be implemented on consumer level with the dialog-ref instance as well. What are your thoughts?

@ibanjo
Copy link
Author

ibanjo commented Aug 5, 2019

Interesting enough, actually. I introspected the A11yDialog class and found it actually fires events on the dialog container. So maybe I'm wrong, but the suggested approach would be something like

// In parent component
this.assignedDialog.on('event-name', callback)

following your idea of managing the actual A11yDialog instance outside the component. While browsing through your code, I've found that maybe it could be more "vue-ish" to follow a component-oriented approach, such as

// Template
<a11y-dialog ref="a11y-comp">...</a11y-dialog>
// JS
this.$refs['a11y-comp'].dialog.on('event-name', callback)

This actually does not leverage your @dialog-ref idea, but results in one less method (i.e. the one in which you assign the dialog-refs) and actually one less object reference, strictly binding the A11yDialog to its component, that is, in my opinion, a better practice in a Vue application.

As a compromise, we may think about proxying the native A11yDialog events in the mounted hook, as in

mounted () {
    this.$nextTick(() => {
        this.dialog = new A11yDialog(this.$refs.rootElement, this.appRoot)

        // Event proxies here
        this.dialog.on('event-name', (data) => this.$emit('a11y-event-name', data))

        this.$emit('dialog-ref', this.dialog)
    })
}

thus allowing the user to simply bind them to the <a11y-dialog> component instance as

<a11y-dialog @event-name="handler"></a11y-dialog>

Shall I try this?

@mgiraldo
Copy link

this would be useful for my use case also

Base automatically changed from master to main February 28, 2021 10:21
this.dialog.hide()
}
},

watch: {
'dialog.shown' (val) {
Copy link
Contributor

@roblevintennis roblevintennis Mar 15, 2022

Choose a reason for hiding this comment

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

Where is dialog.shown coming from exactly?

Also, since this is Vue 3, we should leverage import { ref, watch } from 'vue' idiom and stick it inside the setup (I'd actually like to convert the props to use defineProps as well but didn't quite know about that at the time when I helped with the Vue 3 port).
https://vuejs.org/guide/essentials/watchers.html#basic-example

Another question is how do we deal with when lower-level a11y-dialog opens/closes based off the data attributes not instance?


methods: {
open () {
Copy link
Contributor

Choose a reason for hiding this comment

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

How do/will these play with the case when the consumer asks for the dialog-ref instance? Wouldn't this neuter or create a double callback situation? I suppose both would be handled by the consumer so I suppose they would be silly to both utilize the instance and also listen for the callback. But maybe there's a case where consumer wants to control opening/closing via the instance, but still listen for these events? Also, can the consumer not just do this themselves via the instance 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants