Skip to content

Conversation

@crisbeto
Copy link
Member

@crisbeto crisbeto commented Jun 1, 2018

Disables the CSS-based animations in the overlay when using the NoopAnimationsModule.

Relates to #10590.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 1, 2018
import {OverlayRef} from './overlay-ref';
import {OverlayPositionBuilder} from './position/overlay-position-builder';
import {ScrollStrategyOptions} from './scroll/index';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';
Copy link
Contributor

Choose a reason for hiding this comment

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

This introduces a dependency on @angular/animations that I don't think we want in the CDK

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn’t the token technically in platform-browser? Also we’ve got the experimental/dialog which has a dependency on animations.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is, though I assume @angular/platform-browser/animations would depend on @angular/animations? If it doesn't then maybe this is fine. @jelbourn might know more

@jelbourn
Copy link
Member

jelbourn commented Jun 1, 2018

I'm think we should just leave this one, since we haven't had any issues w/ overlay backdrop in particular

@crisbeto
Copy link
Member Author

crisbeto commented Jun 1, 2018

It’s something that came up in #10590 (comment).

@jelbourn
Copy link
Member

jelbourn commented Jun 1, 2018

I see; let's presubmit it then and see what happens

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 target: patch This PR is targeted for the next patch release labels Jun 1, 2018
@ngbot
Copy link

ngbot bot commented Jun 1, 2018

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "continuous-integration/travis-ci/pr" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mmalerba
Copy link
Contributor

mmalerba commented Jun 1, 2018

@jelbourn the presubmit will probably be fine (maybe have to change a BUILD rule or something) my concern was more, does this change cause @angular/cdk to depend on @angular/animations, and if so is that a problem?

@jelbourn
Copy link
Member

jelbourn commented Jun 1, 2018

Ah, I was just thinking about Google, but yeah, it would technically be a breaking change if there's a new peerDep for @angular/cdk, even though the vast majority of people will have the right packages installed already

@crisbeto
Copy link
Member Author

crisbeto commented Jun 1, 2018

The question is whether it’s actually a new peerDep. The new import is from platform-browser/animations, rather than animations.

@jelbourn
Copy link
Member

jelbourn commented Jun 1, 2018

cdk is currently

  "peerDependencies": {
    "@angular/core": "0.0.0-NG",
    "@angular/common": "0.0.0-NG"
  },

@mmalerba
Copy link
Contributor

mmalerba commented Jun 1, 2018

I asked Matias and he said @angular/platform-browser/animations does depend on @angular/animations. I also put together a stackblitz that shows this: https://stackblitz.com/edit/angular-material2-issue-gul2bw?file=app/app.component.ts

@crisbeto crisbeto force-pushed the 10590/overlay-noop-animations branch from cef381d to b3750f4 Compare June 27, 2018 21:09
@josephperrott josephperrott removed the action: merge The PR is ready for merge by the caretaker label Jun 28, 2018
@ngbot
Copy link

ngbot bot commented Aug 1, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto crisbeto force-pushed the 10590/overlay-noop-animations branch from b3750f4 to 4db7b41 Compare August 3, 2018 15:33
@crisbeto crisbeto force-pushed the 10590/overlay-noop-animations branch from 4db7b41 to c2688ca Compare September 4, 2018 20:45
@crisbeto crisbeto force-pushed the 10590/overlay-noop-animations branch 2 times, most recently from fc219c8 to abec28d Compare September 29, 2018 13:45
@crisbeto crisbeto force-pushed the 10590/overlay-noop-animations branch from abec28d to 6e94dcc Compare November 18, 2018 17:21
@pfeigl
Copy link
Contributor

pfeigl commented Feb 4, 2019

Has there been any decision whether this breaking change is going into cdk? It's easy enough for now using a .cdk-overlay-backdrop { transition: none; } but ugh ... Looking forward to be able to disable transiations in overlays the right way 👍

@mmalerba mmalerba added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Feb 4, 2019
@mmalerba
Copy link
Contributor

mmalerba commented Feb 4, 2019

I think we still want to do this, just at a major release, @jelbourn correct me if I'm wrong

@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
Disables the CSS-based animations in the overlay when using the `NoopAnimationsModule`.

Relates to angular#10590.
@crisbeto crisbeto force-pushed the 10590/overlay-noop-animations branch from 6e94dcc to 601533e Compare June 12, 2019 19:34
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@mmalerba mmalerba added this to the 13.0.0 milestone Jun 24, 2021
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@crisbeto
Copy link
Member Author

Closing in favor of #24687.

@crisbeto crisbeto closed this Mar 29, 2022
@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 Apr 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants