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

Add scss for components in angular apps by default. #2703

Merged
merged 5 commits into from
Jan 10, 2018

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Jan 9, 2018

Issue: #2688

Looks like most of the people (I don't know if it's most, but that what I've seen recently) prefer using scss instead of css.

What I did

Changed the raw-loader to use scss

For components that use styleUrls - scss will be loaded with a raw-loader
For global imports - there will be still style-loader as before.

@igor-dv igor-dv added the angular label Jan 9, 2018
@igor-dv igor-dv self-assigned this Jan 9, 2018
@amcdnl amcdnl self-requested a review January 9, 2018 19:02
Copy link
Member

@amcdnl amcdnl left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Jan 9, 2018

Codecov Report

Merging #2703 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2703   +/-   ##
=======================================
  Coverage   34.36%   34.36%           
=======================================
  Files         390      390           
  Lines        8771     8771           
  Branches      922      892   -30     
=======================================
  Hits         3014     3014           
- Misses       5124     5165   +41     
+ Partials      633      592   -41
Impacted Files Coverage Δ
...p/angular/src/server/config/webpack.config.prod.js 0% <ø> (ø) ⬆️
app/angular/src/server/config/webpack.config.js 0% <ø> (ø) ⬆️
app/vue/src/server/utils.js 0% <0%> (-53.58%) ⬇️
addons/info/src/components/PropTable.js 87.23% <0%> (ø) ⬆️
addons/graphql/src/components/FullScreen/index.js 0% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 33.96% <0%> (ø) ⬆️
...es__/update-addon-info/update-addon-info.output.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Date/index.js 23.52% <0%> (ø) ⬆️
addons/actions/src/lib/retrocycle.js 54.54% <0%> (ø) ⬆️
lib/components/src/table/cell.js 65.21% <0%> (ø) ⬆️
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5e73c4...e00f006. Read the comment docs.

@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 9, 2018

Is it a breaking change? It loaded .css with raw-loader before, and now it doesn't

@amcdnl
Copy link
Member

amcdnl commented Jan 9, 2018

@Hypnosphi - I believe its just for styleUrls if I'm correct.

loader: 'raw-loader',
exclude: /\.async\.(html|css)$/,
exclude: /\.async\.css$/,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to exclude a css file is it doesn't pass the test anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, I think I need to exclude html here.

@Hypnosphi
Copy link
Member

its just for styleUrls if I'm correct

This PR changes styleUrls in example app. Does it mean that users will need to change it as well?

@alterx alterx mentioned this pull request Jan 10, 2018
@igor-dv
Copy link
Member Author

igor-dv commented Jan 10, 2018

@Hypnosphi, it feels like a breaking change, but I prefer to look at it as a bug fix. Before, in order to make css work, people needed to extend webpack.config + exclude thigngs from css rules (because css didn't really work by default) + add scss. So now they maybe will need just to remove these extended webpack.config ...

@Hypnosphi
Copy link
Member

css didn't really work by default

OK then

@Hypnosphi Hypnosphi merged commit 1e8c6dc into master Jan 10, 2018
@Hypnosphi Hypnosphi deleted the angular-support-scss branch January 10, 2018 11:47
@Hypnosphi Hypnosphi restored the angular-support-scss branch January 10, 2018 11:49
@igor-dv igor-dv deleted the angular-support-scss branch January 10, 2018 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants