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.

Sign up for GitHub

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(material/schematics): add option not to include animations module in ng-add #22559

Merged
merged 1 commit into from
Feb 26, 2022
Merged

feat(material/schematics): add option not to include animations module in ng-add #22559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/material/schematics/ng-add/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ describe('ng-add schematic', () => {
describe('animations disabled', () => {
it('should add the NoopAnimationsModule to the project module', async () => {
const tree = await runner
.runSchematicAsync('ng-add-setup-project', {animations: false}, appTree)
.runSchematicAsync('ng-add-setup-project', {animations: 'disabled'}, appTree)
.toPromise();
const fileContent = getFileContent(tree, '/projects/material/src/app/app.module.ts');

Expand Down Expand Up @@ -253,6 +253,20 @@ describe('ng-add schematic', () => {
});
});

describe('animations excluded', () => {
it('should not add any animations code if animations are excluded', async () => {
const tree = await runner
.runSchematicAsync('ng-add-setup-project', {animations: 'excluded'}, appTree)
.toPromise();
const fileContent = getFileContent(tree, '/projects/material/src/app/app.module.ts');

expect(fileContent).not.toContain('NoopAnimationsModule');
expect(fileContent).not.toContain('BrowserAnimationsModule');
expect(fileContent).not.toContain('@angular/platform-browser/animations');
expect(fileContent).not.toContain('@angular/animations');
});
});

describe('custom project builders', () => {
/** Overwrites a target builder for the workspace in the given tree */
function overwriteTargetBuilder(tree: Tree, targetName: 'build' | 'test', newBuilder: string) {
Expand Down
16 changes: 12 additions & 4 deletions src/material/schematics/ng-add/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,18 @@
"x-prompt": "Set up global Angular Material typography styles?"
},
"animations": {
"type": "boolean",
"default": true,
"description": "Whether Angular browser animations should be set up.",
"x-prompt": "Set up browser animations for Angular Material?"
"type": "string",
"default": "enabled",
"description": "Whether Angular browser animations should be included.",
Copy link
Member

Choose a reason for hiding this comment

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

Could we elaborate here on which components strictly require an animations module to function?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but then the option will be somewhat long and the list is bound to go out of date.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the option is really useful without communicating that somehow, though. I wouldn't expect someone to go looking through the source to figure out where it's necessary, and letting someone omit animations just to then get an error when they try to use a component isn't a great experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like that would be too much text to fit in a multi-select list in the command line. We could generate a list of components using animations from the source code and publish it as a URL on material.angular.io that we then link to from the schematic. It might take a while to do it though.

Copy link
Member

Choose a reason for hiding this comment

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

Something like that would probably be useful. I've been meaning to write an "About" page for material.angular.io for a while and a good section there would be "Animations supports"

"x-prompt": {
"message": "Include the Angular animations module?",
"type": "list",
"items": [
{ "value": "enabled", "label": "Include and enable animations" },
{ "value": "disabled", "label": "Include, but disable animations" },
{ "value": "excluded", "label": "Do not include" }
]
}
}
},
"required": []
Expand Down
4 changes: 2 additions & 2 deletions src/material/schematics/ng-add/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ export interface Schema {
/** Name of the project. */
project: string;

/** Whether Angular browser animations should be set up. */
animations: boolean;
/** Whether the Angular browser animations module should be included and enabled. */
animations: 'enabled' | 'disabled' | 'excluded';

/** Name of pre-built theme to install. */
theme: 'indigo-pink' | 'deeppurple-amber' | 'pink-bluegrey' | 'purple-green' | 'custom';
Expand Down
22 changes: 12 additions & 10 deletions src/material/schematics/ng-add/setup-project.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ function addAnimationsModule(options: Schema) {
const project = getProjectFromWorkspace(workspace, options.project);
const appModulePath = getAppModulePath(host, getProjectMainFile(project));

if (options.animations) {
if (options.animations === 'enabled') {
// In case the project explicitly uses the NoopAnimationsModule, we should print a warning
// message that makes the user aware of the fact that we won't automatically set up
// animations. If we would add the BrowserAnimationsModule while the NoopAnimationsModule
Expand All @@ -79,16 +79,18 @@ function addAnimationsModule(options: Schema) {
`because "${noopAnimationsModuleName}" is already imported.`,
);
context.logger.info(`Please manually set up browser animations.`);
return;
} else {
addModuleImportToRootModule(
host,
browserAnimationsModuleName,
'@angular/platform-browser/animations',
project,
);
}

addModuleImportToRootModule(
host,
browserAnimationsModuleName,
'@angular/platform-browser/animations',
project,
);
} else if (!hasNgModuleImport(host, appModulePath, browserAnimationsModuleName)) {
} else if (
options.animations === 'disabled' &&
!hasNgModuleImport(host, appModulePath, browserAnimationsModuleName)
) {
// Do not add the NoopAnimationsModule module if the project already explicitly uses
// the BrowserAnimationsModule.
addModuleImportToRootModule(
Expand Down