remove implicit dependencies on jQuery #55
Conversation
For apps that explicitly remove their dependency on jQuery, this addon will break because it imports from jquery assuming the consuming app already has it. jQuery is imported in the component, but no dependency is declared in the package.json. This works because until recently, jQuery was available packaged with Ember source. [Ember RFC 0386](https://emberjs.github.io/rfcs/0386-remove-jquery.html) propses to remove jQuery eventually, and Ember already ships with the @ember/jquery package installed directly as part of the "ember new" output for new ember apps. related to oskarrough#54
| @@ -1,15 +1,19 @@ | |||
| /* global YT, window */ | |||
|
|
|||
| import $ from 'jquery'; | |||
| }); | ||
| }, | ||
|
|
||
| didInsertElement() { | ||
| this._super(...arguments); | ||
|
|
||
| this.progressBarClick(); |
|
|
||
| // clear the timer | ||
| this.stopTimer(); | ||
|
|
||
| // remove progress bar click handler | ||
|
|
||
| this.removeProgressBarClickHandler(); |
There was a problem hiding this comment.
here i remove the manual click handler event binding.
| @@ -137,21 +145,25 @@ export default Component.extend({ | |||
| // If already loaded, make sure not to load the script again. | |||
| resolve(window.YT); | |||
| } else { | |||
| $.getScript('https://www.youtube.com/iframe_api'); | |||
| let ytScript = document.createElement("script"); | |||
There was a problem hiding this comment.
since ember-ajax and jquery have been removed, $.getScript has been replaced with a native solution borrowed from jQuery.
Under the hood, all jQuery getScript was doing was creating a <script> tag and appending it to the page, thus evaluating the script on page. Here I just do that in 3 lines of code.
| @@ -325,59 +341,73 @@ export default Component.extend({ | |||
|
|
|||
| // OK, this is stupid but couldn't access the "event" inside | |||
| // an ember action so here's a manual click handler instead. | |||
| progressBarClick() { | |||
| addProgressBarClickHandler() { | |||
| this.element.addEventListener( | |||
There was a problem hiding this comment.
without jquery, i just use addEventListener instead of $().on(...).
addon/components/ember-youtube.js
Outdated
| let element = event.srcElement; | ||
| if (element.tagName.toLowerCase() !== "progress") return; | ||
| // get the x position of the click inside our progress el | ||
| let x = event.pageX - element.getBoundingClientRect().x; |
There was a problem hiding this comment.
Replaced jquerys $(this).position().left with the commonly available getBoundingClientRect().x which is now standard across all browsers.
| self.set("currentTime", clickedValue); | ||
| self.send("seekTo", clickedValue); | ||
| }, | ||
| removeProgressBarClickHandler() { |
There was a problem hiding this comment.
the event handler wasn't removed previously, so now it is cleaned up when the component is removed from the DOM.
|
@oskarrough i added some notes describing the changes I made. I hope you'll consider merging this PR as it'll allow Ember users who've removed jquery from their apps to consume your addon safely. |
|
more on Ember removing jquery is here emberjs/rfcs#386 |
|
@oskarrough if you prefer, i can remove the noisy whitespace changes and limit the PR to just the logic changes 🙏 |
|
Thanks so much again, also for the extra comments! Funny how it's now a thing to remove jQuery. Back in the days… if you were really cool you'd have at least three versions of jQuery on the same page ;) Do you mind reverting the Happy to merge in any case. |
|
@oskarrough all set, travis is running now. |
|
released as 0.9.5 🎂 |
Remove jquery
For apps that explicitly remove their dependency on jQuery, this addon will break because it
imports from jquery assuming the consuming app already has it.
jQuery is imported in the component, but no dependency is declared in
the package.json. This works because until recently, jQuery was
available packaged with Ember source.
Ember RFC 0386
propses to remove jQuery eventually, and Ember already ships with the
@ember/jquery package installed directly as part of the "ember new"
output for new ember apps.
related to #54
Remove ember-ajax
I also removed the dependency on
ember-ajaxsince we no longer need this as well.Whitespace/formatting
You'll notice whitespace & formatting fixes, these are autofixes from ESLint.