Removing jQuery from Alert by Johann-S · Pull Request #23596 · twbs/bootstrap

Johann-S · 2017-08-21T12:56:42Z

I'm near the end for this plugin

The last thing I have to fix is :

  $(document).on(
    Event.CLICK_DATA_API,
    Selector.DISMISS,
    Alert._handleDismiss(new Alert())
  )

And how folks will user our plugins without jQuery, maybe we should enable to register our plugins to jQuery even if we do not use it anymore 🤔

Work in progress...
@XhmikosR if you want to help me do not hesitate 😄

Johann-S · 2017-08-21T15:44:18Z

It seems on Windows 8 and 8.1 for IE10 and IE11 CustomEvent produce errors 😢I'll add a polyfill

XhmikosR · 2017-08-21T18:07:44Z

Perhaps we should have a find method for querySelector? It should be faster than querySelectorAll especially if we know we want one element.

BTW, what do you think about adding helper methods around qsa, qs, etc? Instead of us calling the whole thing across the codebase this should result in shorter minified code.

Maybe a findOne method should be a good thing with querySelector and why not adding helpers methods but I prefer longer method because it's simple to know what they do

Do you have any ideas why it failed on IE ? I added a CustomEvent polyfill

I still think we should have our helpers around qsa, qs, etc. The minified code will be a lot shorter that way. We can still name them Util.getElementById etc.

You should make a PR when this one will be merged 👍

Did you try locally with IE?

Found on my local IE thanks 👍 btw I hate to debug on IE 😢

@Johann-S: maybe we could make find accept an element parameter and if missing use document?

Regarding findOne since we already have a function I think it might be better if we just filter the selector and if it's one then call querySelector. For all this we should do some benchmarks because I'm not sure if it's worth in this case. If we called it directly it should be faster for sure.

Yes I agree for your two proposals 👍

Johann-S · 2017-08-21T20:20:35Z

I win against IE 💪 😆

XhmikosR · 2017-08-21T20:31:40Z

IE keeps bugging us :/ Glad you solved it I'll try to have a look tomorrow. BTW perhaps we should have more tests now for these cases.

…

On Aug 21, 2017 23:23, "Johann-S" ***@***.***> wrote: I win against IE 💪 😆 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23596 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVVtTDOIXS1de_d6egv0Hl3AqnnDe8hks5saedMgaJpZM4O9UXM> .

XhmikosR · 2017-08-21T22:16:06Z

+      return null
+    }
+
+    let selectorType = 'querySelectorAll'


After looking into this file I still believe we should have a wrapper generic function and instead do what I suggested and just provide qsa, qs, etc. It's one less thing for the engine to do so it should be as fast possible.

I'm not against it but that's not something to prioritize here we should focus on event namespacing

XhmikosR · 2017-08-21T22:16:45Z

+    var $alert = $('<div class="alert"/>')
+    $alert.appendTo('#qunit-fixture')
+
+    EventHandler.on($alert[0], 'close.bs.alert', function (e) {


So, things still work fine with jQuery?

Yep everything is fine

XhmikosR · 2017-08-21T22:17:37Z

    "js-lint-docs": "eslint --config js/tests/.eslintrc.json assets/js/",
    "js-compile": "npm-run-all --parallel js-compile-*",
-    "js-compile-bundle": "shx cat js/src/dom/event.js js/src/util.js js/src/alert.js js/src/button.js js/src/carousel.js js/src/collapse.js js/src/dropdown.js js/src/modal.js js/src/scrollspy.js js/src/tab.js js/src/tooltip.js js/src/popover.js | shx sed \"s/^(import|export).*//\" | babel --filename js/src/bootstrap.js | node build/stamp.js > dist/js/bootstrap.js",
+    "js-compile-bundle": "shx cat js/src/dom/eventHandler.js js/src/dom/selectorEngine.js js/src/dom/data.js js/src/util.js js/src/alert.js js/src/button.js js/src/carousel.js js/src/collapse.js js/src/dropdown.js js/src/modal.js js/src/scrollspy.js js/src/tab.js js/src/tooltip.js js/src/popover.js | shx sed \"s/^(import|export).*//\" | babel --filename js/src/bootstrap.js | node build/stamp.js > dist/js/bootstrap.js",


Just thinking out loud: this is getting too big. Would it be possible to move this outside of package.json maybe to an external json file file or something?

Yes that would be better if we have other file to add

Yes, but this is just planning ahead. We'll definitely need it and this just seems like bad planning to me.

With my PR which improve our modularisation I introduced a bundler (Rollup) which handle that pretty well

XhmikosR · 2017-08-23T12:15:29Z


      if (selector) {
-        parent = $(selector)[0]
+        const tmpSelected = SelectorEngine.find(selector)


Do we really need this temp var?

No we can remove this var 👍

XhmikosR · 2017-08-23T12:28:14Z

    // static

    static _jQueryInterface(config) {
      return this.each(function () {


I wonder how this still works. each is jQuery specific, isn't it?

Yes but this method is called only when jQuery is present

XhmikosR · 2017-08-23T12:55:30Z

Yup I just saw after I made the comment:) I'm just getting a TypeError in button but we'll see when time comes.

…

On Aug 23, 2017 15:51, "Johann-S" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In js/src/alert.js <#23596 (comment)>: > Util.emulateTransitionEnd(element, TRANSITION_DURATION) } _destroyElement(element) { - $(element) - .detach() - .trigger(Event.CLOSED) - .remove() + EventHandler.trigger(element, Event.CLOSED) + element.parentNode.removeChild(element) } // static static _jQueryInterface(config) { return this.each(function () { Yes but this method is called only when jQuery is present — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23596 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVVtZX1v0TCaGXs3KUosfxhl5t6rj5hks5sbCBhgaJpZM4O9UXM> .

Johann-S added js v4 labels Aug 21, 2017

Johann-S added 2 commits August 21, 2017 15:04

Near the end of removing jQuery from Alert

2985fb6

fix unit test

2a5a34d

Johann-S force-pushed the v4-alert-without-jquery branch from 448b4a9 to 2a5a34d Compare August 21, 2017 14:25

better polyfill for closest and matches functions

96e025c

Johann-S force-pushed the v4-alert-without-jquery branch from 8e1d401 to 96e025c Compare August 21, 2017 14:51

Johann-S force-pushed the v4-alert-without-jquery branch from 2c0c0d4 to 28b8564 Compare August 21, 2017 17:50

XhmikosR reviewed Aug 21, 2017

View reviewed changes

Add CustomEvent polyfill and a working preventDefault for IE

9b65b23

Johann-S force-pushed the v4-alert-without-jquery branch from 28b8564 to 9b65b23 Compare August 21, 2017 20:09

XhmikosR reviewed Aug 21, 2017

View reviewed changes

Johann-S force-pushed the v4-alert-without-jquery branch from b353ca1 to e9b95e9 Compare August 23, 2017 09:06

Add namespaced events

438f06b

Johann-S force-pushed the v4-alert-without-jquery branch from e9b95e9 to 438f06b Compare August 23, 2017 09:22

Add event delegation + fix EventHandler.one

48dbae6

Johann-S force-pushed the v4-alert-without-jquery branch 2 times, most recently from 87dd8dd to b91d0c8 Compare August 23, 2017 10:29

XhmikosR reviewed Aug 23, 2017

View reviewed changes

Remove jQuery from alert.js and add .alert only if jQuery is available

f862b8e

Johann-S force-pushed the v4-alert-without-jquery branch from b91d0c8 to f862b8e Compare August 23, 2017 13:58

Johann-S merged this pull request into v4-without-jquery Aug 23, 2017

Johann-S deleted the v4-alert-without-jquery branch August 23, 2017 14:05

Johann-S mentioned this pull request Aug 23, 2017

v5 without jQuery #23586

Merged

45 tasks

Uh oh!

Conversation

Johann-S commented Aug 21, 2017

Uh oh!

Johann-S commented Aug 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Johann-S commented Aug 21, 2017

Uh oh!

XhmikosR commented Aug 21, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XhmikosR Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XhmikosR commented Aug 23, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

XhmikosR Aug 21, 2017 •

edited

Loading