-
Notifications
You must be signed in to change notification settings - Fork 34
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
2.0 preview #64
2.0 preview #64
Conversation
b0e0818
to
e02513f
Compare
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
8f6398f
to
d093013
Compare
CLAs look good, thanks! |
53a0b1e
to
36a7f23
Compare
The demo seems broken: nothing happens when I click on any of the buttons or the boxes labelled 'target'. |
demo/index.html
Outdated
@@ -96,7 +94,7 @@ | |||
</simple-fit> | |||
<script> | |||
var defaultTarget = Polymer.dom(myFit).parentNode; | |||
var template = document.querySelector('template[is="dom-bind"]'); | |||
var template = document.querySelector('dom-bind'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work in 1.x?
@bicknellr re nothing happening on click => Polymer/polymer#4509 |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
button.selected { | ||
background-color: #b3e5fc; | ||
} | ||
</style> | ||
<template is="dom-bind"> | ||
<dom-bind><template is="dom-bind"> | ||
<template is="dom-repeat" items="[[containers]]"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this dom-repeat
require a wrapper? I know that you don't need them within a dom-module
's template but I can't remember if they're necessary in another dom-*
's template like this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the demo's working so this one must not require a wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, Kevin says that template extensions don't need wrappers if they're nested in a template that's being managed by Polymer in some way (i.e. the template for a dom-module
or anywhere in the dom-*
elements`).
iron-fit-behavior.html
Outdated
setTimeout(function() { | ||
// If invisible or in a shadow root that's still not connected, wait a tick. | ||
if (window.getComputedStyle(this).display === 'none' || | ||
document.body.contains(this) === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document.body.contains(this)
will only be true when this
isn't inside any ShadowRoot, even if its connected. Are you finding that attached
is getting fired when this
isn't connected? If so, this could be another reactions queue issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that the element wasn't centered correctly on firefox. After debugging i realized that it had all 0s in its getClientBoundingRect
on attached
. I should debug this more and discover where's the issue exactly - I was under the impression that attached
would be called in cases like this
var ce = document.createElement('custom-element');
document.createElement('div').appendChild(ce);
// div not in body, though custom-element is attached!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attached
only called in gets called as a result of connectedCallback
(I don't think there are other places?) and connectedCallback
reactions are only enqueued when an element's shadow-including root changes from a non-document to a document. So, running that snippet on its own won't cause ce
'sattached
to run. However, if attached
is getting called as a result of that snippet, then this is likely a bug in the custom elements polyfill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. I'll write a small repro & file a bug then 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a different behavior compared to 1.x - awaiting news on webcomponents/shadydom#120
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to flush in b6f5441
iron-fit-behavior.html
Outdated
this.fit(); | ||
}.bind(this)); | ||
} else { | ||
// NOTE: shadydom won't apply the upgrades synchronously for performance reasons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same comment as this one in paper-ripple, this is problem is about distribution rather than upgrades)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default positionTarget
used by the IronFitBehavior element in the demo is different in 1.x vs 2.x because dom-bind
seems to be putting things inside it next to the template in 1.x and outside itself in 2.x. This makes the demo seem really broken, particularly in 1.x since the dom-bind
ends up with display: inline;
(the default for unknown elements). I'll confirm with Kevin, but this should probably be considered a dom-bind
bug. If not, the demo needs to be restructured to make this look consistent.
Wow good catch! 💯 Yeah it looks like a |
Filed as Polymer/polymer#4536; sounds like they're planning on fixing this today. |
^ Update on this bug: merged, so should be in |
... and we're tagged. I'll give the demos another quick check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, demo looks good with Polymer v1.9.1!
CLAs look good, thanks! |
I adapted it to work both in 1.0 and 2.0.