Skip to content

[modal] change event order #454

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

Merged
merged 2 commits into from
May 26, 2017
Merged

[modal] change event order #454

merged 2 commits into from
May 26, 2017

Conversation

youaresofunny
Copy link
Contributor

There are no issue about it (but i think we can discus this little fix here)

The chain of events in hide method of modal now looks like this:
click smth (ok / cancel) -> trigger(change, hidden) -> trigger(ok/cancel)
It means that 'hidden' (or 'change') is triggered even before we start to process the 'ok' or 'cancel' methods. (so if we e.cancel() in ok, we will anyway see hidden trigger)

After commit it will be work in this way:
click smth (ok / cancel ) -> trigger(ok / cancel) -> trigger(change) -> trigger(hidden) (if event not canceled)

Also, can i add "beforechange" emit, that triggers before "ok" / "cancel" ?

@alexsasharegan
Copy link
Member

So I think there's been some discussion on this in the past. I'm in favor of complying with the existing BS4 events emitted by the modal:
image

It is important to note that this can't be done with just calling these events in a different order—transition-group hooks have to be implemented. An added difficulty bonus here is that the transition group hooks are called once per transition element. We currently have two transition elements.

@youaresofunny
Copy link
Contributor Author

Well, I'll just add BS4 event system there

@pi0 pi0 requested a review from alexsasharegan May 25, 2017 22:05
@tmorehouse
Copy link
Member

tmorehouse commented May 25, 2017

Also, Vue.js (and boostrap-vue) is reactive based, whereas Bootstrap V4 native is event based. so there are a few differences in how things can be handled.

Although I do think having some of the added events fire on $root would be handy... The biggest issue is providing context to the event when it is fired. i.e. providing the ID of the component, a reference to the component, or an event object (with target and maybe relatedTarget attached).

@alexsasharegan
Copy link
Member

@youaresofunny You can see part of the long-running questions on issue #276

@youaresofunny
Copy link
Contributor Author

Yep, but this commit is not about $root emits 🚶‍♂️

@alexsasharegan
Copy link
Member

You're right, @youaresofunny , I got carried away on another topic. The order of events seems more sensible with your changes. They don't take into account the asynchronous nature of the transitions, but that is probably best dealt with in a later, breaking change. Now with your event renaming, I don't believe we've made any breaking changes at all, and you should have your event flow worked out.

Any extra thoughts, @tmorehouse ?

@tmorehouse
Copy link
Member

I wonder if we should standardize the format / calling signature of all component emits on $root?

i.e. this or something like this:

this.$root.$emit('event::name', this.id, this, other, args, here)

this.$root.$on('event::name', id, ctx, other , args, here)

Although not all components have an id, so maybe just this and the receiving handler could look for a ctx.id?

@tmorehouse
Copy link
Member

tmorehouse commented May 26, 2017

For local vm.$emits() I don't see too much of an issue with the proposed order.

the transition triggered events might be something we have to deal with later.

@alexsasharegan
Copy link
Member

@tmorehouse, with all those options, why not make a class and just pass that? I've got some sweet class patterns you guys might like, so pass me the prop names and I'll PR the internal event object so we can define an interface and avoid too many parameters.

@tmorehouse
Copy link
Member

The modal click handler (where it can be canceled) is kind of nifty, although it is binding to a native event structure ($event) which we don't have the luxury of in $emits. So maybe modeling after the native event might be handy, or we could just create an event from new Event and take advantage of that. although it might have too many props we don't need.

I can see possibly providing 'target' being the VM of the component, and relatedTarget could be a reference, if needed to, another element.

Maybe event.target (native element ref), event.$target being the component VM reference, etc.

@alexsasharegan
Copy link
Member

Yeah, I was thinking of more of a hybrid event. A class that uses a lot of the same Event names, but it more scoped to the internal needs of the BV components. For example, the emitting component would be placed on target, and the modal canceler would be done with a closure trick using a preventDefault callback on the SyntheticEvent.

@alexsasharegan
Copy link
Member

If you've played with react, they have done some cool things with their synthetic event to make it familiar, but ergonomic. We have very small needs, but I thought that was a good target to emulate.

@tmorehouse
Copy link
Member

Cool. I think it should have props similar to cancellable and defaultPrevented, etc.

relatedTarget, could be good as well (and optionally set, null otherwise)

@alexsasharegan alexsasharegan requested a review from pi0 May 26, 2017 04:25
@pi0 pi0 merged commit e5f4253 into bootstrap-vue:master May 26, 2017
@tmorehouse tmorehouse changed the title modal: change event order [modal] change event order Jun 23, 2017
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