Replacing jQuery with native methods#59
Conversation
Converted using ember-native-dom-helpers-codemod
|
I've raised a PR to chrislopresto/ember-freestyle#157 that removes freestyle's internal dependency on jQuery. Once it lands, it'd be safe to update to that version while this PR uses the branch of that PR. |
sivakumar-kailasam
force-pushed
the
remove-jquery-dependency
branch
from
6983d06 to
696e874
Compare
|
@Kerrick the previous failure was because of a lack of changes on .travis.yml. Its fixed now. |
|
Thanks for the PR! Definitely in favor of this strategy. Once code review has passed we can merge & ship this 🎉 |
danielraggs
left a comment
danielraggs
left a comment
There was a problem hiding this comment.
I left you one question, but otherwise everything looks good.
| getFocusableElements: () => this.$(FOCUSABLE_ELEMENTS), | ||
| hasNecessaryDom: () => !!get(this, 'element') && !!this.$(ITEMS_SELECTOR).length, | ||
| getFocusableElements: () => this.element.querySelectorAll(FOCUSABLE_ELEMENTS), | ||
| hasNecessaryDom: () => Boolean(this.element), |
There was a problem hiding this comment.
Just curious, I might be missing something here but what happened to the ITEMS_SELECTOR?
There was a problem hiding this comment.
This was the check in their default adapter, so went with it. Also we're guaranteed that the necessary dom exists if the element gets rendered in this case.
Kerrick
added
Status: Awaiting Reply
Status: In Progress
and removed
Status: Pending
Status: Awaiting Reply
labels
|
Looks like the only thing left is for chrislopresto/ember-freestyle#157 to get merged & published so this PR's branch can point to a published version of ember-freestyle. |
|
@Kerrick this is good to merge now. |
|
@sivakumar-kailasam Your PR is almost good to go; however, I did some quick testing and found that the MDC-menu component has a bug in your branch: once the menu is opened, you're not able to click outside to close the menu. After you fix this bug your PR will be good to merge! 👍 |
|
@danielraggs Sure will take a look at that and see if I can add a test case for it while take a stab at it 😄 |
|
@danielraggs added a fix but couldn't add a test for it yet. I hope to add acceptance tests when I raise a different PR to update the deps. |
- ➕ Add prettier.js (#67) - ➕ Add prettier.js (#62) - ➖ Replace jQuery with native methods (#59) - ✨ Bind DOM events to some components (#61) - 🐛 Don't set foundations on destroyed components (#60) - 🚑 Fix CI autodeploy - :busts_in_sillhouette: Add Contributor - 🚀 AutoDeploy docs to GitHub Pages on merge to master (#58) - 💚 Temporary Travis CI workaround - 👥 Add contributor - ⬆️ [docs] Upgrade Ember Composable Helpers - 🐛 Fix #52 - 📝 Fix contributors (#53) - ⬆️ Upgrade Ember CLI to 2.18.0 - 🐛 If no label is present show the placeholder - 🐛 Move ember-freestyle to devDependencies - ✨ Implement {{mdc-drawer}} (#45)
3 participants
Many apps decide to exclude jQuery and it makes it impossible if addons use them. Its good that material components themselves don't care about jQuery. This is PR removes all dependency on jQuery.