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

Include addon styles for MU apps #8289

Merged
merged 6 commits into from Feb 1, 2019

Conversation

ppcano
Copy link
Contributor

@ppcano ppcano commented Dec 13, 2018

  1. The PR supersedes use module unification layout for styles #8134.

This change fixes the CSS stylesheets that require importing from addons feature for MU applications importing styles from classic addons.

  1. This PR also supersedes Fix MU addon styles #8146 to allow MU applications importing SASS modules like the ember-styleguide.

MU applications will now execute preprocessCss on packageStyles like Classic applications and it removes the existing preprocessCss call of the processSrc method.

I am not sure this is the correct implementation but this implementation looks more natural and it enables the specifying custom output paths works properly test at 6c65235 (cc @twokul).

A similar issue was reported at #8197 where MU applications executed preprocessTemplates twice on processSrc and processTemplates.

Closes #8134, #8133, #8146

@ppcano
Copy link
Contributor Author

ppcano commented Dec 13, 2018

@patricklx I think the commit 183269e in this PR fixes the same issue as #8146. I will continue investigating because I think the problem is in the pre-processor logic.

@ppcano ppcano mentioned this pull request Dec 13, 2018
@ppcano ppcano force-pushed the addon-styles-mu-fix branch 3 times, most recently from e8df6d8 to 32e53b7 Compare December 13, 2018 23:15
@NullVoxPopuli
Copy link
Contributor

anything left on this?

@ppcano
Copy link
Contributor Author

ppcano commented Dec 25, 2018

No, as far as I know.

@ppcano
Copy link
Contributor Author

ppcano commented Jan 22, 2019

@NullVoxPopuli who can finally review or merge this one?

This PR fixes a few reported bugs and change slightly the current MU preprocess logic.

@NullVoxPopuli
Copy link
Contributor

I don't remember if I've tested it, but, @ppcano, this repo should work when pointed at your branch: https://github.com/NullVoxPopuli/test-ember-paper-with-module-unification 🤷‍♂️ can't test it out atm.
package.json is somewhat old: https://github.com/NullVoxPopuli/test-ember-paper-with-module-unification/blob/master/package.json#L25

@NullVoxPopuli
Copy link
Contributor

but, I'd say either @rwjblue or @Turbo87 ?

@Turbo87
Copy link
Member

Turbo87 commented Jan 23, 2019

sorry, I don't know enough about MU to be able to review this properly

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor changes needed here (to make future debugging of this pipeline easier), but overall looks great!

lib/broccoli/ember-app.js Show resolved Hide resolved
lib/broccoli/default-packager.js Outdated Show resolved Hide resolved
lib/broccoli/default-packager.js Show resolved Hide resolved
lib/broccoli/default-packager.js Show resolved Hide resolved
@ppcano
Copy link
Contributor Author

ppcano commented Feb 1, 2019

@rwjblue Done

@lolmaus
Copy link
Contributor

lolmaus commented Feb 3, 2019

Thx, @ppcano! 🙇

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.

None yet

6 participants