Skip to content

Removing jQuery from Carousel#23658

Merged
Johann-S merged 2 commits intov4-without-jqueryfrom
v4-carousel-without-jquery
Sep 5, 2017
Merged

Removing jQuery from Carousel#23658
Johann-S merged 2 commits intov4-without-jqueryfrom
v4-carousel-without-jquery

Conversation

@Johann-S
Copy link
Copy Markdown
Member

@Johann-S Johann-S commented Aug 24, 2017

Rewrite our Carousel plugin without jQuery

  • Improve off method of our EventHandler to remove listeners in an entire namespace

Work in progress...

@Johann-S Johann-S requested a review from XhmikosR August 24, 2017 20:24
@Johann-S Johann-S changed the title Carousel without jQuery Removing jQuery from Carousel Aug 24, 2017
@Johann-S Johann-S mentioned this pull request Aug 24, 2017
45 tasks
@Johann-S Johann-S force-pushed the v4-carousel-without-jquery branch from 219f4cf to caf1d50 Compare August 25, 2017 09:27
Comment thread js/src/util.js
}
},

extend(obj1, obj2) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not use Object.assign?
I thing another issue is that this implementation is deepCopy but $.extend by default is not recursive.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If Object.assign cannot be used I would use arguments (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments) and then you can change Util.extend(Utils.extend... to Util.extend(obj1, obj2, obj3, ...)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't had time to think more of this implementation of extend but it did the job 😄
I'll take a look thanks @wojtask9 👍
If you want you can submit a PR which target this branch (v4-carousel-without-jquery) with a better implementation

@Johann-S Johann-S force-pushed the v4-carousel-without-jquery branch 6 times, most recently from 6a60409 to 2533d79 Compare August 31, 2017 10:40
@Johann-S Johann-S force-pushed the v4-carousel-without-jquery branch 3 times, most recently from 8afec53 to 540e2c7 Compare September 3, 2017 13:57
@Johann-S Johann-S force-pushed the v4-carousel-without-jquery branch from 540e2c7 to 579a6f7 Compare September 3, 2017 15:17
@Johann-S
Copy link
Copy Markdown
Member Author

Johann-S commented Sep 3, 2017

last issue here, when I call dispose the first time all the event listeners were removed, but if click on the next arrow (a new carousel is created) and if I call dispose again the listeners are alive 😢

@Johann-S Johann-S force-pushed the v4-carousel-without-jquery branch 2 times, most recently from 626b0f7 to dce1cb3 Compare September 5, 2017 09:52
@Johann-S
Copy link
Copy Markdown
Member Author

Johann-S commented Sep 5, 2017

I should improve my off method to allow to remove all the event but not only for namespaced events for example :

EventHandler.off(element, 'click')

// Currently
EventHandler.off(element, 'bs.carousel')
EventHandler.off(element, 'click', myHandler)

@Johann-S Johann-S force-pushed the v4-carousel-without-jquery branch from dce1cb3 to ce87c76 Compare September 5, 2017 10:54
@Johann-S
Copy link
Copy Markdown
Member Author

Johann-S commented Sep 5, 2017

All test passed except one from our Button plugin but it's a known issue

@Johann-S Johann-S merged commit ee104f8 into v4-without-jquery Sep 5, 2017
@Johann-S Johann-S deleted the v4-carousel-without-jquery branch September 5, 2017 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants