Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observe nodes #73

Merged
merged 4 commits into from
Oct 28, 2015
Merged

Observe nodes #73

merged 4 commits into from
Oct 28, 2015

Conversation

cdata
Copy link
Contributor

@cdata cdata commented Oct 14, 2015

This change does the following:

/cc @notwaldorf

This branch is blocked by Polymer/polymer#2548 being tagged. It is currently released on polymer#master, but until it is tagged these changes cannot be merged into iron-selected#master.

@cdata
Copy link
Contributor Author

cdata commented Oct 14, 2015

/cc @azakus

@@ -306,11 +284,6 @@
this._updateSelected();
}
}.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can factor away the bind here if you want. The callback is called in the context of the observed node and you shouldn't need the argument now.

@sorvell
Copy link
Contributor

sorvell commented Oct 15, 2015

Can we add a test that would have failed before? For example, add a node that only gets to distributed to a child of the selector via re-projection?

@sorvell
Copy link
Contributor

sorvell commented Oct 15, 2015

Why don't we change the items property so that it's only calculated when we get a signal that the items may have changed (startup + when observedNodes tells you).

@cdata
Copy link
Contributor Author

cdata commented Oct 16, 2015

Feedback addressed. Tests are failing locally due to a bug in observeNodes, reported offline to @sorvell

@cdata
Copy link
Contributor Author

cdata commented Oct 16, 2015

Also, I threw in a fix for #69 since it was relevant and it felt lame to fix it with the old code.

@cdata
Copy link
Contributor Author

cdata commented Oct 16, 2015

add a node that only gets to distributed to a child of the selector via re-projection?

We already appear to have tests for this that passed. See: https://github.com/PolymerElements/iron-selector/blob/master/test/content.html

Hrm..

@cdata
Copy link
Contributor Author

cdata commented Oct 17, 2015

Already resolved, but this change now depends on resolution of Polymer/polymer#2587

@notwaldorf
Copy link
Contributor

Just a note for future @cdata: You'll need to bump the Polymer version up in bower.json to 1.2.0

@cdata cdata force-pushed the observe-nodes branch 2 times, most recently from 83f4d63 to 247c23c Compare October 28, 2015 19:44
cdata added a commit that referenced this pull request Oct 28, 2015
@cdata cdata merged commit 92b0bbc into master Oct 28, 2015
@cdata cdata deleted the observe-nodes branch October 28, 2015 20:06
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.

3 participants