-
Notifications
You must be signed in to change notification settings - Fork 24
Resolve relative sass_root paths earlier #65
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
base: main
Are you sure you want to change the base?
Resolve relative sass_root paths earlier #65
Conversation
59d2803 to
f82915f
Compare
f82915f to
2684cf3
Compare
|
I'll look later this week, no immediate feedback except: i'd still start with a ".sass" check :) |
|
Yeah sorry, I ignored this part on purpose and I took the conservative choice to only rollback SassCssCompiler. |
|
I mande a PR on your PR with small change suggestions :) |
…rlier Resolve relative sass_root paths earlier SymfonyCasts#65
…esolve-relative-sass-path-earlier
bocharsky-bw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
Should we merge now? Or is there still any pending work to do? |
|
Based on the thread I think it's still missing tests. If I remember correctly, I didn't know (and still don't know) how to properly test these changes. |
Revert SassCssCompiler before #64. Supports relative paths by resolving/prefixing them at extension loading. I don't know if it should be implemented this way (it feels hacky?) but I don't see how it could be better.
What do you think of this change @smnandre ?
Tests are missing because I don't know enough how asset-mapper works to design an integration test for an AssetCompiler. Testing the builder instead would be even more work for me because my dev env doesn't supports the default sass binary from the bundle.
I'll not have more time in the next weeks to work on it, feel free to modify this PR as much as you want.