Skip to content

Conversation

devversion
Copy link
Member

  • Currently the schematics add the custom-theme SCSS code to any project style file (styles.EXT). This is incorrect because the SCSS needs to be placed inside of a SCSS file. Instead of throwing an error we just create a new SCSS file with the theme and add it to the workspace config.
  • Aligns the global font-family statement with the current Angular Material typography font-family.
  • Adds missing tests for the global app styles.
  • Removes an accidentally committed console.log().

@devversion devversion added pr: merge safe target: major This PR is targeted for the next major release labels Aug 17, 2018
@devversion devversion requested a review from amcdnl as a code owner August 17, 2018 07:44
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 17, 2018
* Currently the schematics add the custom-theme SCSS code to any project style file (`styles.EXT`). This is incorrect because the SCSS needs to be placed inside of a SCSS file. Instead of throwing an error we just create a new SCSS file with the theme and add it to the workspace config.
* Aligns the global font-family statement with the current Angular Material typography font-family.
* Adds missing tests for the global app styles.
* Removes an accidentally committed `console.log()`.
@devversion devversion force-pushed the fix/ng-add-theme-custom-css branch from 5770507 to 43f5b19 Compare August 17, 2018 07:50
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -16,6 +17,14 @@ describe('material-install-schematic', () => {
runner = new SchematicTestRunner('schematics', collectionPath);
});

/** Expects the given file to be in the styles of the specified workspace project. */
function expectProjectStyleFile(project: WorkspaceProject, filePath: string) {
expect(project.architect!['build']).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

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

Could reduce repetition:

const architect = project.architect!;
expect(architect['build']).toBeTruthy();
...

@devversion devversion added the action: merge The PR is ready for merge by the caretaker label Aug 21, 2018
@jelbourn jelbourn merged commit 51da6a6 into angular:master Aug 21, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants