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

@IncludeStyles does not work on non-root component #89

Closed
akaegi opened this issue May 2, 2019 · 13 comments
Closed

@IncludeStyles does not work on non-root component #89

akaegi opened this issue May 2, 2019 · 13 comments
Assignees

Comments

@akaegi
Copy link
Contributor

akaegi commented May 2, 2019

Somehow @IncludeStyles does not seem to work on a non-root component.
See my sample code at: /~https://github.com/akaegi/include-styles-demo.

(dom-module 'app-colors' defined in index.html)

@asyncLiz asyncLiz self-assigned this May 2, 2019
@asyncLiz
Copy link
Collaborator

asyncLiz commented May 2, 2019

Seems like the component factory is not available when I'm scanning for it. A quick workaround is to add the components using @IncludeStyles to the entryComponents of their module.

That fixes for JIT compilation, but neither component has the styling with AOT compilation. I'm going to have to dig into that one further. Not sure if that's an existing bug, or something with the newer versions of Angular.

@asyncLiz
Copy link
Collaborator

asyncLiz commented May 2, 2019

I've got a fix for AOT not working, but we come back to the problem of the component factory.

We could document and require that any component using @IncludeStyles needs to be added to the entryComponents. Not a huge fan of that though, since it'll get annoying with many components and it adds extra overhead in creating factories that aren't needed.

We really just need to associate an HTML selector string with one or more Polymer style modules. The only reason I'm using a decorator and component factory is so that it improves the developer experience. There's no reason we can't do something like this:

IncludeStyles.for('app-child')('app-colors');

@Component({
  selector: 'app-child'
})
export class ChildComponent {}

It feels "side effect"-y to me, but it'd solve the problem and could get rid of a lot of code that is trying to dynamically look up the selector provided to @Component.

Using a decorator "feels" much better, but I can't think of a clever way to link a class's type to it's component metadata without the component factory.

If you have any suggestions, I would appreciate it!

@akaegi
Copy link
Contributor Author

akaegi commented May 7, 2019

Thank you for your time and inputs! I digged further into the issue and it turned out it is a different issue.
I created another demo at /~https://github.com/akaegi/include-styles-demo2 to illustrate it.

I use vaadin components so I'm not yet sure if the problem might be on their side...
I use a custom-style to include 'lumo-color' and 'lumo-typography' as global style. In combination with using vaadin-tabs this removes the style element from the 'lumo-color' dom-module. (See screenshots). This is ShadyCSS code but seems to be ok?
As a consequence, the @IncludeStyles has the effect of adding an empty style element to my Angular component when using it for 'lumo-color'.

Is the solution that you have to apply some ShadyCSS functions from within your @IncludeStylescode? (The style is still there in the dom-module element but "hidden" within the _styles field).

Hope that explanation is not too confusing...

Note that when the global style-include for 'lumo-color' is deactivated (app.component.ts line 10), then the dom-module style is not "erased" and the @IncludeStyles works as expected)

image

image

@asyncLiz
Copy link
Collaborator

asyncLiz commented May 7, 2019

Good find! Looks like cssFromTemplate() does remove the style node from the DomModule element. That makes sense, since we don't want those styles leaking in legacy browsers.

I could change IncludeStyles to use cssFromModules() to get the CSS string instead of manually querying it.

@akaegi
Copy link
Contributor Author

akaegi commented May 7, 2019

That looks right to me on first sight (but I am a real novice what concerns Polymer and ShadyCSS).
Do you have to handle the case when ShadyCSS is not available?

@asyncLiz
Copy link
Collaborator

asyncLiz commented May 7, 2019

ShadyCSS doesn't touch Polymer style modules, so there's no step there I have to worry about. Origami already checks and adds stylesheets to ShadyCSS in another module. All IncludeStyles does is add the string to the component styles, which would get passed through ShadyCSS later.

@akaegi
Copy link
Contributor Author

akaegi commented May 9, 2019

Seems to work fine :-)
Changed "style retrieval" in patchedRendererFactory to:

const styleModules = getStyleModulesFor(getTypeFor(selector));
      let styles = styleModules.map(styleModule => {
        const styleElements = stylesFromModule(styleModule);
        return styleElements.map(e => e.innerText).join('\n');
      });

where stylesFromModule is imported from '@polymer/polymer/lib/utils/style-gather.js.
(cssFromModules is deprecated).

Shall I issue a PR?

@asyncLiz
Copy link
Collaborator

asyncLiz commented May 9, 2019

Sure! PRs are always welcome. If we don't use importStyleModule() anymore, we'll want to add @deprecated warnings to it so that it can be removed in the next major version

@asyncLiz
Copy link
Collaborator

AOT fix is pushed out and will be published shortly, but I'm leaving this open for the other problem

@akaegi
Copy link
Contributor Author

akaegi commented Jun 18, 2019

For the style-related issue I issued a PR some time ago. Did you have the chance to look at it so that it can possibly be merged?

@asyncLiz
Copy link
Collaborator

Looks like my only comments on it were documentation, I can merge and tweak those manually.

@akaegi
Copy link
Contributor Author

akaegi commented Jul 9, 2020

I'm not sure why this issue is still open. My PR was merged a long time ago (#90)...

@asyncLiz
Copy link
Collaborator

asyncLiz commented Jul 9, 2020

I left this open for some reason, but I can't think of it now!

@asyncLiz asyncLiz closed this as completed Jul 9, 2020
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

No branches or pull requests

2 participants