Fix SASS compilation issues #8013
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are several ways add-on can add a style to the application:
+ create `app/styles` folder
+ create `addon/styles` folder
+ return a tree from `treeForStyles` hook
Previously, some add-ons (when using `treeForStyles` hook) would return
content under `app/styles`. That used to work before packager changes.
Because there's no contract (interface) that Ember CLI provides when you return
a tree from `treeForStyles` hook, packager treats it as an opaque tree
and moves its contents later on the "right" place. It unfortunately
creates scenarios as such:
```
the-best-app-ever/
styles/
app/
styles/
```
where nested `app/styles` is a tree from an add-on.
While I believe add-ons should return content under the "root" of the
tree, we do need to support current behaviour w/o breaking anything.
Useful links:
+ `app/styles` folder in
[`ember-basic-dropdown`](https://github.com/cibernox/ember-basic-dropdown/tree/v1.0.3/app/styles)
+ `treeForStyles` hook in
[`ember-cli-tailwind`](https://github.com/embermap/ember-cli-tailwind/blob/v0.6.1/index.js#L67)
+ `treeForStyles` hook in
[`ember-bootstrap`](https://github.com/kaliber5/ember-bootstrap/blob/v0.11.3/index.js#L54)
kellyselden
reviewed
Sep 5, 2018
| let addons = this.addonTreesFor('styles'); | ||
|
|
||
| let flatten = addons.map(tree => | ||
| new Funnel(tree, { |
There was a problem hiding this comment.
Can you add comments in here about stuff to deprecate as people are doing things we don't want?
kellyselden
approved these changes
Sep 5, 2018
rwjblue
approved these changes
Sep 5, 2018
|
Merging (so we can get the release out), @twokul would you mind following up with the comments that @kellyselden mentioned? |
|
@rwjblue absolutely |
|
It still doesn't include addon styles that are imported from the app.import("app/styles/style.css")Note: It works if I move it to the |
|
@kanongil I don't think |
|
Yes, I realise that (and I have moved my styles), but it's not documented that way, and it's done in the wild. |
|
AFAIK |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are several ways add-on can add a style to the application:
app/stylesfolderaddon/stylesfoldertreeForStyleshookPreviously, some add-ons (when using
treeForStyleshook) would returncontent under
app/styles. That used to work before packager changes.Because there's no contract (interface) that Ember CLI provides when you return
a tree from
treeForStyleshook, packager treats it as an opaque treeand moves its contents later on the "right" place. It unfortunately
creates scenarios as such:
where nested
app/stylesis a tree from an add-on.While I believe add-ons should return content under the "root" of the
tree, we do need to support current behaviour w/o breaking anything.
Useful links:
app/stylesfolder inember-basic-dropdowntreeForStyleshook inember-cli-tailwindtreeForStyleshook inember-bootstrap