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

Rewrite templating #93

Merged
merged 14 commits into from
Jun 26, 2016
Merged

Rewrite templating #93

merged 14 commits into from
Jun 26, 2016

Conversation

Saulis
Copy link
Owner

@Saulis Saulis commented Jun 19, 2016

Fixes #8
Fixes #56
Fixes #74
Fixes #88
Fixes #89
Fixes #90
Fixes #92


This change is Reviewable

@web-padawan
Copy link
Contributor

web-padawan commented Jun 23, 2016

I've got an error then using this branch:

[iron-data-table::_annotatedComputationEffect]: compute method `or` not defined

Computed function or is defined in a behavior used by my element which includes iron-data-table.
It seems that computed functions are not processed correctly.

@Saulis please take a look.

@web-padawan
Copy link
Contributor

Update: computed functions bug is still reproduced, but only then iron-data-table is wrapped into dom-if template.
E. g. we have multiple iron-ajax requests, and we don't want to show table until them all become completed, as items array is collected from their results.

@Saulis Saulis force-pushed the rewrite-templatizers branch from b31bae4 to 2bf7c88 Compare June 24, 2016 06:17
@Saulis
Copy link
Owner Author

Saulis commented Jun 24, 2016

@web-padawan thanks for the update, I've forced push a new version, please try if it works any better.

@web-padawan
Copy link
Contributor

web-padawan commented Jun 24, 2016

Redefining rootDataHost seems to be correct now, but for some reasons polymer is now walking through DOM far too much when seeking for behaviors.

Before:

[iron-data-table::_annotatedComputationEffect]: compute method `or` not defined

Now:

[bi-projects-page::_annotatedComputationEffect]: compute method `or` not defined

I'm not able to provide a real example due to complexity (and the page is still WIP), but the bi-projects-page is more than one level up from the component which includes iron-data-table.

@web-padawan
Copy link
Contributor

Another issue is related to data bindings from outside (see button text) which are being only applied to the first row.
datatable

@Saulis
Copy link
Owner Author

Saulis commented Jun 24, 2016

Right, I see. I'll try to have a better look tomorrow. Would be awesome if you provide me with some isolated example code!

@web-padawan
Copy link
Contributor

Thanks. I'll try to reproduce this issue tonight in a separate repo for easier debugging.

@web-padawan
Copy link
Contributor

web-padawan commented Jun 24, 2016

I created a sandbox, you can see it in action here, and the code including iron-data-table is here.

  • The bug with first line button is reproduced
  • behaviors bug is reproduced too (see Status column)
[sandbox-demo-page::_annotatedComputationEffect]: compute method `equal` not defined

As you can see, behavior is expected to present in sandbox-demo-page - which is one level top from sandbox-demo-table-sub-page actually implementing this behavior.

@Saulis please pay attention to that template element is used to detect rootDataHost.
I'm trying to guess if this bug can be related to my usage of iron-lazy-pages element, which implements templatizer behavior itself.

@Saulis Saulis force-pushed the rewrite-templatizers branch 2 times, most recently from 0e91627 to 2a2d704 Compare June 25, 2016 19:50
@Saulis Saulis force-pushed the rewrite-templatizers branch from 2a2d704 to c0b2ec6 Compare June 25, 2016 21:16
@Saulis
Copy link
Owner Author

Saulis commented Jun 25, 2016

@web-padawan huge thanks for the sandbox! I think I was able fix the issues, assigning the root datahost follows now more closely to the default implementation. Oh and the bindings [[equal(item.isActive, 'true')]] were missing hyphens around the true if you happen to wonder why the equal method isn't executed when binding.

Please check out the latest version of rewrite-templatizers

@Saulis Saulis force-pushed the rewrite-templatizers branch from c0b2ec6 to 66835fe Compare June 25, 2016 21:23
@web-padawan
Copy link
Contributor

@Saulis Thanks a lot!
I updated the sandbox and replaced the method used in bindings to ensure it is called correctly.
All works fine with hidden attribute binding, as well as with dom-if templates.

@Saulis
Copy link
Owner Author

Saulis commented Jun 26, 2016

Glad to hear! Thanks for your help on this issue, I'll go ahead and merge the changes.

@Saulis
Copy link
Owner Author

Saulis commented Jun 26, 2016

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.


data-table-templatizer-behavior.html, line 13 [r6] (raw file):

      template: Object,

      //singleton

_instances and _forwardedParentProps being singletons can be problematic when there are multiple instances of <iron-data-table> which have templates bound to different parent properties that have the same name.


Comments from Reviewable

@Saulis Saulis merged commit 17a1a0d into master Jun 26, 2016
@Saulis Saulis deleted the rewrite-templatizers branch June 26, 2016 10:05
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