search

PolymerElements / paper-icon-button

A Material Design icon button
https://www.webcomponents.org/element/PolymerElements/paper-icon-button
42 stars 44 forks source link

Fix #145 - Append template to head instead of body #146

Closed dejan9393 closed 5 years ago

dejan9393 commented 5 years ago

As stated in title

evancaldwell commented 5 years ago

I made the same change in my code and it works great. We are currently broken on this issue so this PR would help us a lot.

bicknellr commented 5 years ago

Are you compiling your app to ES5 and then loading it as a classic script (not type="module")?

cc @graynorton, a lot of the 3.0 elements seem to be doing different things for getting their templates in place: some are doing this, some are appending to document.head, others are passing them on the object given the Polymer function as _template (which seems like the best option).

dejan9393 commented 5 years ago

@bicknellr in my case, I'm serving both es6 and an es5 transpiled version (depending on browser support).

The app that I ran into this bug on is not a Polymer app - i'm hacking my way around including Polymer Components using a faux-entrypoint file (generated by Polymer, included in the actual entrypoint with a build step).

The problem arises due to the fact that I try to load my WebComponents early, and other render-blocking script tags that are included afterwards delay the render just long enough for the paper-icon-button to attempt to append its template to the <body> tag.

So yes, included as a non-module <script> tag, but it's es6 code.

evancaldwell commented 5 years ago

I'm bundling an es5 version with webpack and including that with a classic script but are soon going with an un-transpiled es6 version (still bundled with webpack). Not sure what the hold up is on this. Seems like all the other paper/iron elements are appending to head.

bicknellr commented 5 years ago

After some offline discussion here, I don't think that this qualifies as a release blocker for the 3.0 elements since this is valid behavior for modules (as opposed to classic scripts) and you can get the same timing as modules by adding the defer attribute to your classic script. (See step 26 of "prepare a script" in the HTML spec for more detail.)

For now, I'd like to keep changes to the element repos limited to only things that would make them unusable (with the expectation that they're run as modules with specifiers resolved before reaching the browser) so that I can continue iterating through prereleases to be sure they're all stable when running together without introducing changes that might cause unexpected regressions.

However, I don't see any reason we can't make this improvement after they've been tagged as stable. It doesn't look like this would cause any significant problems - I just don't want to test that assumption at this point in the process.

evancaldwell commented 5 years ago

@bicknellr I understand your position here but it presents an issue for us even using defer because we have a pretty complex build with multiple bundles so defering that work causes problems where certain things (like the marked library for marked-element) aren't defined. I guess I'm still not understanding why this PR can't be merged in. It's a very small change, most of the other paper/iron elements append to the head anyway and, at least in our case, it does make this element unusable.

evancaldwell commented 5 years ago

I see that this was moved to 3.0.0. Can this PR be revisited?

bicknellr commented 5 years ago

Yeah, we can start looking at these now. Also, I'm going to change the base branch to master so we can delete __auto_generated_3.0_preview.

frasun commented 5 years ago

+1 for looking at this 🙂

dejan9393 commented 5 years ago

I don't believe this is required anymore, as PR #149 has addressed this in a different way (released in v3.0.2)