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: respect animation @.disabled binding #12829

Closed

Conversation

devversion
Copy link
Member

For custom CSS transitions we currently only check whether the NoopAnimationsModule is used or not. If noop animations are used, we apply a CSS class that can be used to set the transition or animation to none.

Since developers can toggle the animations dynamically through the [@.disabled] binding (which goes down the component tree), we should also respect this binding because such binding is commonly used.

Also instead of using a mixin that does magic, we should explicitly manage the selectors in order to keep the generated CSS as predictable and small as possible.

See https://angular.io/api/animations/trigger and #12812 (comment)

Note: Technically shouldn't be a breaking change. More a fix for developers using @.disabled.

@devversion devversion added the target: major This PR is targeted for the next major release label Aug 25, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 25, 2018
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 Aug 25, 2018
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@devversion
Copy link
Member Author

Note: There are a few more components that use custom transitions but don't respect the NoopAnimationsModule at all. I will work on a follow-up PR that makes them consistent.

@ben-yocum
Copy link

Nice work. This will be very helpful to me.

When you talk about fixing "a few more components" in a later PR, which ones do you mean? For example, mat-menu does not respect [@.disabled] and has caused problems for my E2E tests.

devversion added a commit to devversion/material2 that referenced this pull request Aug 26, 2018
* Ensures that all components with custom CSS transitions respect the `NoopAnimationsModule` or `[@.disabled]` state.

Related to angular#12829
@devversion
Copy link
Member Author

@gitsage There are a few components which don't respect the NoopAnimationsModule at all.

With #12839, those components should be consistent with other components which already respect the NoopAnimationsModule. Also those will respect the [@.disabled] binding (like with this PR)

devversion added a commit to devversion/material2 that referenced this pull request Aug 26, 2018
* Ensures that all components with custom CSS transitions respect the `NoopAnimationsModule` or `[@.disabled]` state.

Related to angular#12829
@ngbot
Copy link

ngbot bot commented Sep 18, 2018

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

3 similar comments
@ngbot
Copy link

ngbot bot commented Sep 18, 2018

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

@ngbot
Copy link

ngbot bot commented Sep 18, 2018

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

@ngbot
Copy link

ngbot bot commented Sep 18, 2018

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

@devversion devversion force-pushed the feat/respect-at-disabled-state branch from 375634d to dd3881e Compare September 19, 2018 09:53
@devversion devversion added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Oct 2, 2018
@devversion devversion force-pushed the feat/respect-at-disabled-state branch from dd3881e to b28d273 Compare October 4, 2018 15:33
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Oct 4, 2018
@devversion devversion added target: patch This PR is targeted for the next patch release target: minor This PR is targeted for the next minor release and removed target: major This PR is targeted for the next major release target: patch This PR is targeted for the next patch release labels Oct 19, 2018
@devversion devversion force-pushed the feat/respect-at-disabled-state branch from b28d273 to 44473cc Compare November 10, 2018 08:53
For custom CSS transitions we currently only check whether the `NoopAnimationsModule` is used or not. If noop animations are used, we apply a CSS class that can be used to set the `transition` or `animation` to `none`.

Since developers can toggle the animations dynamically through the `[@.disabled]` binding (which goes down the component tree), we should also respect this binding because such binding is commonly used.

Also instead of using a mixin that does magic, we should explicitly manage the selectors in order to keep the generated CSS as predictable and small as possible.
* It can happen that someone applies `[@.disabled]` directly on the component host element (e.g. `<mat-tab-group [@.disabled]="disableAnimations"]></mat-tab-group>`)
@devversion devversion force-pushed the feat/respect-at-disabled-state branch from 44473cc to 905d620 Compare November 16, 2018 17:22
@josephperrott josephperrott added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Mar 4, 2019
@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@devversion
Copy link
Member Author

@jelbourn This never got merged and now has a lot of conflicts. do you think it's worth rebasing this?

I don't think it is a priority, but maybe rather something we need to be aware of when creating the MDC protoypes?

@mmalerba
Copy link
Contributor

Yeah we should make sure to do this for the MDC-based components, should be easy since they're still in experimental. If you're going to rebase this one I would suggest splitting it into separate PRs per component

@devversion
Copy link
Member Author

@mmalerba 👍 Regarding rebasing this PR: I'm not sure it's worth doing that as it probably requires a new audit to look for new instances that need to be updated.

@jelbourn
Copy link
Member

Yeah, at this point I think we can skip this change and ensure we set things up property for the MDC components. One thought there, though, is that we support more granular animation options instead of it being all or nothing. I've heard from some users that they want to e.g. disable animations for menus, but not for progress-bar/spinner

@devversion devversion closed this Aug 28, 2019
@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 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

7 participants