Skip to content

Conversation

@b-laporte
Copy link
Member

This PR adds the support of CSS selectors as $getElement argument. This had to be done through brute force (i.e. by cloning the DOM elements in a document fragment) as components are not obliged to create a single container element, where a simple call to querySelector() could be done - cf. cloneEltForSelector() in the tnode file.

@benouat
Copy link
Member

benouat commented Jul 15, 2014

I might be missing something, and I know that component are not forced to declare a single parent element, but in the end, every html node always has a parent. so we could simply keep a reference of only one of the root nodes and apply the querySelector() on it ?

@b-laporte
Copy link
Member Author

@benouat Well if we run querySelector() on the component parent's node, we might get references to elements that are not in the component (e.g. reference to an element of another component instance in the same parent..). Having said that, another possible algorithm could have been to do a querySelectorAll() and then bubble from each result to find out if they are in the right DOM hierarchy..

@benouat
Copy link
Member

benouat commented Jul 15, 2014

we might get references to elements that are not in the component

You see I was missing a point ! you're right, it can not do the trick.

For the other algorithm, let's keep it as it is today. If we ever encountered performance issue, we then could try it...

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) when pulling 7c14b80 on b-laporte:refresh-qselector into a223a0b on ariatemplates:master.

@PK1A
Copy link
Contributor

PK1A commented Jul 15, 2014

This PR would be a breaking change if I'm reading it correctly. If this is the case I think we should really start to be a little bit more strict about our commit message conventions and the changelog generation.

Personally I would suggest sticking to https://github.com/ajoslin/conventional-changelog which allow us to signal breaking changes and clearly indicate those in the changelog. WDYT?

@benouat
Copy link
Member

benouat commented Jul 16, 2014

I am also in favor of using https://github.com/ajoslin/conventional-changelog it makes it very clear.

@b-laporte
Copy link
Member Author

I am fine with the change log tool that you propose - but in this precise case, the change is backward-compatible as previous interface is still supported. As such I wouldn't call it a 'breaking change', even though it adds significant added value to the developer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants