refactor(jQlite): Add the ability to use query selector without jQuery - #15986
refactor(jQlite): Add the ability to use query selector without jQuery#15986orafaelfragoso wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
There was extensive discussion about making .find(selector) work and we decided against it as it was impossible to defer to querySelectorAll in a simple way that will agree with jQuery (& common sense) due to a design bug of querySelectorAll with respect to running from non-document elements. But these arguments don't extend to root-level selectors so I can see angular.element(selector) working in JQLite... I just want to make it clear we're not going to support .find(selector). Thanks for the PR! |
Sorry, something went wrong.
|
@rafaelfragosom Please make sure grunt test passes for you locally, there are some errors. |
Sorry, something went wrong.
|
@mgol I agree, .find(selector) is too ugly for such a small DOM query. I only want to be on the same page here. Are these changes wellcome or should I close this issue? Is querySelector an option or should I use a different approach? I can see the compatibility at almost 100% on caniuse. Let me know so I can put some more effort on this. Thank you. |
Sorry, something went wrong.
|
@rafaelfragosom I consulted the team and we're OK with making angular.element(selector) work without jQuery but .find(selector) should still fail. As long as those conditions are met, we will accept a PR. We'll need unit tests that confirm both of those conditions are met. Would you be willing to work on that? |
Sorry, something went wrong.
| 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.
You need to use window.document. We don't assume browser globals are available globally, they have to be taken from window. You can also see that the Travis build failed because of that.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for letting me know.
I've been busy this entire time but I'll take the time to finish the PR.
Sorry, something went wrong.
There was a problem hiding this comment.
I added the window call to it and rebased everything.
Sorry, something went wrong.
`angular.element` threw an error if we tried to use query selector
directly 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 and
to 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
mgol
left a comment
There was a problem hiding this comment.
I think this needs refactoring, I added some comments.
Also, we need unit tests for all new functionality. Can you add some?
Sorry, something went wrong.
| 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.
This is not a good place to add this logic as it'd mean new angular.element(selector) wouldn't work. This whole if should just be removed and the logic should me moved down.
Sorry, something went wrong.
There was a problem hiding this comment.
Also, note it should use querySelectorAll, not querySelector as we want to select all matching elements, not one.
Sorry, something went wrong.
|
@rafaelfragosom Hey, are you still interested in finishing this PR? |
Sorry, something went wrong.
|
We're now in LTS mode so no new features are accepted. Changing the milestone. |
Sorry, something went wrong.
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.element threw an error if we tried to use query selector
directly 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 and
to get the jQlite wrapper with the goodies.
Now we can do this, without jQuery:
angular.element('.something')
And get the same result.