-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat(ng-add): respect project default style extension #12618
feat(ng-add): respect project default style extension #12618
Conversation
* No longer ignores the projects default style extension that has been specified in the `ng new` command (e.g. `new new test --style scss`)
@@ -47,4 +44,17 @@ describe('material-dashboard-schematic', () => { | |||
`import { MatGridListModule, MatCardModule, MatMenuModule, MatIconModule, MatButtonModule } from '@angular/material';`); | |||
}); | |||
|
|||
it('should support passing the style extension option', () => { | |||
const tree = runner.runSchematic('dashboard', {styleext: 'scss', ...baseOptions}, | |||
createTestApp()); |
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.
small nit: prefer line breaking at the higher syntactic level
const tree =
runner.runSchematic('dashboard', {styleext: 'scss', ...baseOptions}, createTestApp());
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.
Makes sense - I've started the wrap after the function name. That's what we do within other Jasmine tests as well (e.g. expectTo(
) and that's also what CLang proposed me to do.
@@ -41,7 +41,11 @@ describe('material-install-schematic', () => { | |||
'./node_modules/@angular/material/prebuilt-themes/indigo-pink.css'); | |||
}); | |||
|
|||
it('should add custom theme', () => { | |||
it('should support adding a custom theme', () => { | |||
// TODO(devversion): currently a "custom" theme does only work for projects using SCSS. |
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.
Does the CLI not support mixing scss with other preprocessors?
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.
It does. The problem with the install
schematic right now is that we also just add the custom theme to projects that don't use SCSS.
The schematic does not create a new scss file in a plain css project. It just looks for the default styles.(s)css
file and puts the code in there. This is incorrect because the CSS files don't support the SASS syntax.
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.
LGTM
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
ng new
command (e.g.new new test --style scss
)