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

feat(schematics): add shell schematic #9883

Merged
merged 4 commits into from Feb 18, 2018
Merged

Conversation

amcdnl
Copy link
Contributor

@amcdnl amcdnl commented Feb 10, 2018

This PR adds a schematic for wiring up Angular Material and all of its dependencies in a Angular CLI application.

@amcdnl amcdnl self-assigned this Feb 10, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 10, 2018
@amcdnl amcdnl requested a review from jelbourn February 10, 2018 18:23
@@ -0,0 +1,5 @@
# Angular Material Schematics
A collection of Schemaatics for Angular Material.
Copy link

Choose a reason for hiding this comment

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

Schemaatics -> typo

"schematics": {}
"schematics": {
"materialShell": {
"description": "Create a Material shell",
Copy link
Member

Choose a reason for hiding this comment

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

Could you expand the description to something like "Adds Angular Material to an application without changing any templates"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record, comments are technically not supported in JSON. ;P

Copy link
Contributor

Choose a reason for hiding this comment

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

The collection.json are read as JSON5; http://json5.org/

@@ -1,5 +1,12 @@
// This is the root config file where the schematics are defined.
{
"$schema": "./node_modules/@angular-devkit/schematics/collection-schema.json",
"schematics": {}
"schematics": {
"materialShell": {
Copy link
Member

Choose a reason for hiding this comment

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

FYI we're probably going to change this name at some point but it's still TBD

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ng-add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding an alias for it gives this error: Error: Schematics/alias "ng-add" collides with another alias or schematic name.

# Material Shell
Adds Angular Material and its depedencies and pre-configures the application.

It does the following:
Copy link
Member

Choose a reason for hiding this comment

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

You could omit the "It does the following: " ; the list by itself is good

* - Adds pre-built themes to styles.ext
* - Adds Browser Animation to app.momdule
*/
export default function(options: Schema): Rule {
Copy link
Member

Choose a reason for hiding this comment

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

Do you have to use a default export? These are generally strongly discouraged / banned inside Google (and these schematics will someday be used inside Google)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansl - All of the schematics seem to use default exports (ref: https://github.com/angular/devkit/blob/master/packages/schematics/angular/app-shell/index.ts#L313). Thoughts on @jelbourn comment?

const themeName = options && options.theme ? options.theme : 'indigo-pink';
const themeSrc = `../node_modules/@angular/material/prebuilt-themes/${themeName}.css`;
const hasCurrentTheme = app.styles.find((s: string) => s.indexOf(themeSrc) > -1);
const hasOtherTheme = app.styles.find((s: string) =>
Copy link
Member

Choose a reason for hiding this comment

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

Prefer breaking at the higher syntactic level:

const hasOtherTheme = 
    app.styles.find((s: string) => s.indexOf('@angular/material/prebuilt-themes') > -1);

return (host: Tree) => {
const config = getConfig(host);
config.apps.forEach(app => {
const themeName = options && options.theme ? options.theme : 'indigo-pink';
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making it create a custom theme file by default? I believe that most people will want to customize the theme to one of the non-standard options, so it will be one less step when they want to swap it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, I'll add an option for custom theme :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelbourn - Custom theme requires SCSS be enabled. If they don't have this do we want to configure it for them automatically?

export const angularVersion = '5.0.0';
export const materialVersion = '^5.2.0';
export const cdkVersion = '^5.2.0';
export const angularVersion = '^5.0.0';
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to make this ^5.2.0?

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.

Just a couple more nits

// Define the palettes for your theme using the Material Design palettes available in palette.scss
// (imported above). For each palette, you can optionally specify a default, lighter, and darker
// hue. Available color palettes: https://www.google.com/design/spec/style/color.html
$candy-app-primary: mat-palette($mat-indigo);
Copy link
Member

Choose a reason for hiding this comment

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

Can we get the real name of the app in here instead of candy-app?


// Define the palettes for your theme using the Material Design palettes available in palette.scss
// (imported above). For each palette, you can optionally specify a default, lighter, and darker
// hue. Available color palettes: https://www.google.com/design/spec/style/color.html
Copy link
Member

Choose a reason for hiding this comment

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

We should add a link to the theming guide here in comments

/**
* Add pre-built styles to style.ext file
*/
function addImportToStyles(options: Schema) {
Copy link
Member

Choose a reason for hiding this comment

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

addThemeToAppStyles?

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

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Feb 18, 2018
@jelbourn jelbourn merged commit 45399c6 into angular:master Feb 18, 2018
@amcdnl amcdnl deleted the shell-schematic branch February 18, 2018 21:35
@andrewseguin andrewseguin added the target: minor This PR is targeted for the next minor release label Feb 20, 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 8, 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: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants