fixes explicit uses of 'a-scene' #1665
Conversation
|
I'd submitted a PR on this already, but it had extraneous stuff in it. So, I cleaned out my repository, re-applied the changes an resubmitted. |
|
It looks good. The only concern is some problems we had in the past that might not be issues anymore. When using HTML templates the isScene might not be available (because the custom element is not yet initialized) while you can still check for a tagName. @ngokevin @cvan Do you have any use case in mind where these changes could break things? |
|
|
||
| if (targetSelector) { | ||
| this.targetEls = this.closest('a-scene').querySelectorAll(targetSelector); | ||
| this.targetEls = this.el.sceneEl.querySelectorAll(targetSelector); |
There was a problem hiding this comment.
We can probably leave this one since it's deprecated, just to make sure we don't break it
|
Don't think there will be much side effects. HTML templates are no longer used. And the changes here look like |
3 participants
Description:
A number of places in the source assumed that the only possible scene would be an 'a-scene' element. In most places in the code, however, the .sceneEl (property on any element contained in a scene) and .isScene (property on a scene) are used. By directly using 'a-scene' (when searching for elements, and so on), we cannot create new kinds of scenes.
Changes proposed:
replace different references with existing alternatives (el.sceneEl, el.isScene)
add new closestScene() method to a-node.js, and use it instead of closest('a-scene')
add new findAllScenes() method to utils.js, and use it instead of document.querySelectorAll('a-scene');