Rewritten modal without jquery

[alekitto]

To make it work correctly the following changes were required:

• Trigger events in jQuery if available
• Add util functions for manipulating data attributes and jQuery data object
• Util.extend now supports multiple objects

[Johann-S]

Why did you add a new regex here ? off method already work

[Herst]

const does not work in IE10

[XhmikosR]

We use babel.

[alekitto]

The off implementation was not complete and i think i've messed up some things. Now working on a cleaner and more jQuery-like solution.

[Johann-S]

here you check for jQuery !== undefined it should be better with $ which come from your Util.jQuery getter

[alekitto]

You're right. Fixed.

[Johann-S]

why do you call stopImmediatePropagation here ? evt isn't dispatched

[Johann-S]

setDataAttribute and removeDataAttribute should be in our DOM Manipulator here : https://github.com/twbs/bootstrap/blob/v4-without-jquery/js/src/dom/manipulator.js

[Johann-S]

Without a change here, our Modal continue to depend on jQuery

[alekitto]

I've missed it. I've just changed it.

[Johann-S]

You made a very good work here @alekitto but they are few changes here before I can merge your PR 👍

[Johann-S]

Not a big fan here because EventHandler.getBrowserTransitionEnd() will be called each time Util.supportsTransitionEnd() is called

[Johann-S]

I updated the base branch with v4-dev, so that's why they are other commits

[alekitto]

Ok, i've rebased my branch with upstream. Now working on your comments..

[Johann-S]

Here you add a check if transition is set or not, you should revert this change it was better before that, because they aren't any check done each time we call this method

[alekitto]

Cyclic import (util -> eventHandler -> util) was causing me troubles yesterday, but now seems working correctly. Probably it was another error.

[Johann-S]

if you want you can use EventHandler here

[Johann-S]

Oups last thing I forgot, can you update our visual modal test ? please

[alekitto]

Ok, EventHandler off method has been rewritten to support namespaced events (click.namespace), native events without namespace (click removes click.namespace) and namespace only (.namespace removes click.namespace and keydown.namespace).

Visual modal test has been updated

[Johann-S]

I don't understand why you made that change on off because off already support namespaced events 🤔
What's the difference between what you've done and what we had ? and why you did that change ?

EDIT :
So if we want to call off on a namespaced event like that : click.bs.alert, is it working ?

[Johann-S]

that's not our button plugin 😄

[alekitto]

Whoops

[alekitto]

The off method did not remove events if event type like click.dismiss.bs.modal was passed. This would lead to events not removed in the hide method, especially from document and window nodes. Additionally, non namespaced events was removed only if an handler function was specified as third argument

[Johann-S]

Ok thank you for the explanation 👍 and yep the second point wasn't implemented thank you

[alekitto]

Yes, now EventHandler.off(element, 'click.bs.alert') is working as expected

[arcanedev-maroc]

About Modal events, can i suggest a feature request ?

How about adding a public method to listen a modal event:

class Modal {
    //...

    on(event, callback) {
        this._element.addEventListener(`${event}${EVENT_KEY}`, callback);    
    }

    //...
}

So instead of:

let myModal = new bootstrap.Modal(document.getElementById('myModal'));
myModal._element.addEventListener('hidden.bs.modal', function (e) {
    // do something...
})

We can do something like this:

var myModal = new bootstrap.Modal(document.getElementById('myModal'));
myModal.on('hidden', function (e) {
    // do something...
});

I don't know if it's going to have a conflict with jquery but we can change the method name on into something else like listen('hidden', ...).

I can create a PR for this feature if it's OK 👍

[alekitto]

My 2 cents: This could be an interesting feature, but I think that should be implemented for all the components.
For example:

const myCollapse = new bootstrap.Collapse(document.getElementById('test-collapse'));
myCollapse.on('shown', e => { ... });

could be equally good and useful.

@arcanedev-maroc probably you should open a new issue proposing a these changes to the event system of the components to better discuss pros and cons of this new design.

[arcanedev-maroc]

@alekitto DONE 👍

Issue: #31266