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

Added attributes angularEquals and onlyAngularToPolymer #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added attributes angularEquals and onlyAngularToPolymer #42

wants to merge 1 commit into from

Conversation

martinsik
Copy link

Hi! First of all you're mixng tab and space indentation which makes quite a mess in the code. Some lines have also spaces at the end. I think you should refactor it and use only spaces.

I already used this extension in a real app. I'm using <core-list> to render very large 5000*20 table. Therefore I had to add two new attributes that controll Angular's $watch call.

  • angularEquals tells ng-polymer-elements whether you really want to use $watch(attr, handler, true) which uses angular.equal() internaly for object equality instead of comparing object references (basically one-way data binding). That's all right untill you're using <core-list> which is an absolute overkill and basicaly makes it useless.
  • onlyAngularToPolymer this specifies that you're not going to modify the watched variable inside the Polymer element so you don't need to use angular.copy() when the value changes (I'm actually not even sure if you need to make the copy anyway) and also you don't need to watch for changes with Polymer's PathObserver. Using angular.copy() makes such overhead that makes <core-list> almost useless.

If the attribute names are weird just rename them. I think these are both useful features and the default functionality remains unchanged when not using them at all. Also, if you merge this commit maybe mention them in README.md?

@motin
Copy link

motin commented Nov 15, 2015

@GabiAxel, @martinsik Any update on this? Looks like a healthy PR.

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.

2 participants