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

Fix: ember-component-css compatibility #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

buschtoens
Copy link

Fixes #8.

However, this is blocked by: webark/ember-component-css#75

@xomaczar
Copy link
Owner

Agree. Out of the box we should satisfy 99% of use cases w/o any custom config.
Reloading *.css (vendor & app) is straight forward and was the initial implementation, however I thought parsing vendor css (which most of the time does not change during the dev and may contain large css framework) was unnecessary.

@buschtoens
Copy link
Author

That's right - if only we wouldn't have ended up with just the opposite: only the vendor.css reloading. 😄

But the rationale is right. Would be ignoring the vendor.css too much of a trade-off? As you said, the vendor.css updates very rarely; practically only after a new addon is installed (afaik).

Maybe we can just leave it up to the user to do these reloads, if any, manually, to save us a big headache.

@xomaczar
Copy link
Owner

Well, the only reason we ended up with the opposite is due to using non ember-cli prescribed app structure. It works correctly in all other cases. To ignore vendor.css requires knowing that a given file will be concatenated into vendor.css - same issue as we have now with {app}.css

@buschtoens
Copy link
Author

How about having these (default) config settings:

{
  reload: {
    app: true,
    vendor: false
  }
}

This way the user can decide, whether or not they need vendor.css reloading, which I would assume most do not. I'd have no other idea on how to teach this addon when to reload which file.

@xomaczar
Copy link
Owner

@buschtoens your input is valuable and greatly appreciated. Can you please elaborate what happens when reload {app:false, vendor:true} and user modifies app/styles/app.sass. Do we just reload vendor.css because a style was changed?

@buschtoens
Copy link
Author

I'd say yes.

The problem is, we can't possibly know the behaviour of all the addons out there. But I haven't yet encountered a single addon that imported from the app directory into the vendor.css tree. But that need not mean that there's not such an addon. However, I'd go by the default assumption, that app only contains "app" code. If users somehow get vendor code mangled in there, they are free to set vendor: true and have it work. All other 99 or so % would be satisfied with the default behavior.

Another thing though: There might be a way to know, when a file belongs to the vendor.css tree. We could hook in to the app.import(asser, options) call. However, this could potentially slow down boot time and is quite hacky.

In the end, I would really just ignore the vendor.css stuff completely by default, but allow users to enable it on demand. At least in my workflow, I'd never need it. I'd just hit F5, if ever needed.

@xomaczar
Copy link
Owner

Sounds great. Do you have time to make PR with associated test that support this new workflow. If not, I can probably get to it at the end of this week.

@buschtoens
Copy link
Author

Sorry for not responding. I was on vacation for a few days. 🍹

I guess you also didn't have the time to prepare a commit? Maybe I'll get something cooked up tonight or tomorrow.

@xomaczar
Copy link
Owner

xomaczar commented Aug 1, 2015

I am currently vacationing as well. Try to find time this week

@xomaczar
Copy link
Owner

@buschtoens interested in making PR per our discussion?

@erkie
Copy link

erkie commented Aug 31, 2015

I have no idea where this currently is but I tried out the current branch in my project and it worked awesomely!

I'm using:

"ember-cli": "1.13.8",
"ember-cli-sass": "^4.0.1",
"ember-cli-styles-reloader": "[email protected]:buschtoens/ember-cli-styles-reloader.git#fix-ember-component-css-compat",
"ember-component-css": "0.1.6"

@xomaczar
Copy link
Owner

xomaczar commented Sep 1, 2015

@erkie @buschtoens branch fix-ember-component-css should work for you. Based on our discussion above, we decided to simplify that implementation to support various app styles locations.

@buschtoens
Copy link
Author

Yup. Maybe I'll get around to it this evening. 😊

Andrey Khomenko [email protected] schrieb am Di., 1. Sep. 2015
15:00:

@erkie https://github.com/erkie @buschtoens
https://github.com/buschtoens branch fix-ember-component-css should
work for you. Based on our discussion above, we decided to simplify that
implementation to support various app styles locations.


Reply to this email directly or view it on GitHub
#9 (comment)
.

@xomaczar
Copy link
Owner

xomaczar commented Sep 1, 2015

@buschtoens thx, sounds great.

@erkie
Copy link

erkie commented Jan 26, 2016

Any status on this? :D

@erkie
Copy link

erkie commented Jan 26, 2016

@xomaczar I can't find the branch fix-ember-component-css anywhere? I'd be willing to help solve this issue.

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.

3 participants