- Notifications
You must be signed in to change notification settings - Fork27.4k
refactor(jQlite): Add the ability to use query selector without jQuery#15986
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was extensive discussion about making But these arguments don't extend to root-level selectors so I can see Thanks for the PR! |
@rafaelfragosom Please make sure |
@mgol I agree, I only want to be on the same page here. Are these changes wellcome or should I close this issue? Is Let me know so I can put some more effort on this. Thank you. |
@rafaelfragosom I consulted the team and we're OK with making We'll need unit tests that confirm both of those conditions are met. Would you be willing to work on that? |
src/jqLite.js Outdated
| @@ -284,7 +284,7 @@ function JQLite(element) { | |||
| } | |||
| if (!(this instanceof JQLite)) { | |||
| if (argIsString && element.charAt(0) !== '<') { | |||
| throw jqLiteMinErr('nosel', 'Looking up elements via selectors is not supported by jqLite! See: http://docs.angularjs.org/api/angular.element'); | |||
| return new JQLite(document.querySelector(element)); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You need to usewindow.document. We don't assume browser globals are available globally, they have to be taken fromwindow. You can also see that the Travis build failed because of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for letting me know.
I've been busy this entire time but I'll take the time to finish the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added thewindow call to it and rebased everything.
ff4f3a1 to389ecf4Compare`angular.element` threw an error if we tried to use query selectordirectly without jQuery. I thought that this was a silly littlefix for the framework.A lot of the projects that I've worked forthe past years are only including jQuery to use query selectors onthe app (I know that are options for this), and I'm only making this PRbecause I think it's unnecessary to use`angular.element(document).find()` to accomplish such a small thing andto get the jQlite wrapper with the goodies.Now we can do this, without jQuery:`angular.element('.something')`And get the same result.use the window to call document as instructed
389ecf4 toecb0569CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this needs refactoring, I added some comments.
Also, we need unit tests for all new functionality. Can you add some?
| @@ -284,7 +284,7 @@ function JQLite(element) { | |||
| } | |||
| if (!(this instanceof JQLite)) { | |||
| if (argIsString && element.charAt(0) !== '<') { | |||
| throw jqLiteMinErr('nosel', 'Looking up elements via selectors is not supported by jqLite! See: http://docs.angularjs.org/api/angular.element'); | |||
| return new JQLite(window.document.querySelector(element)); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is not a good place to add this logic as it'd meannew angular.element(selector) wouldn't work. This wholeif should just be removed and the logic should me moved down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, note it should usequerySelectorAll, notquerySelector as we want to select all matching elements, not one.
@rafaelfragosom Hey, are you still interested in finishing this PR? |
We're now in LTS mode so no new features are accepted. Changing the milestone. |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
refactor
What is the current behavior? (You can also link to an open issue here)
angular.element('.my-selector')throws an error if jQuery is not included.What is the new behavior (if this is a feature change)?
angular.element('.my-selector')now finds the element and wraps it in a jQlite (or jQuery) object.Does this PR introduce a breaking change?
No
Please check if the PR fulfills these requirements
Other information:
angular.elementthrew an error if we tried to use query selectordirectly without jQuery. I thought that this was a silly little
fix for the framework.
A lot of the projects that I've worked for
the past years are only including jQuery to use query selectors on
the app (I know that are options for this), and I'm only making this PR
because I think it's unnecessary to use
angular.element(document).find()to accomplish such a small thing andto get the jQlite wrapper with the goodies.
Now we can do this, without jQuery:
angular.element('.something')And get the same result.