refactor(modal): allow to disable dialog animation#1478
refactor(modal): allow to disable dialog animation#1478
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a global flag to disable modal animations, which is a good improvement for accessibility and performance on low-end devices. The implementation correctly uses a helper to check for the global setting.
However, the current implementation is incomplete. The global animation setting is only applied to the animation timings controlled by animationTime(), but not to the CSS-based fade animations or the backdrop animation logic.
I've added a couple of suggestions to refactor the logic into a reusable isAnimated property. Adopting this suggestion and using the new property consistently in showBackdrop() and the component's template (si-modal.component.html) will fix the issue and make the code cleaner.
|
Documentation. Coverage Reports: |
spliffone
force-pushed
the
refactor/modal-allow-disabling-animations
branch
from
466bd67 to
e3fa53f
Compare
|
|
||
| private animationTime(millis: number): number { | ||
| return this.modalRef?.data.animated !== false ? millis : 0; | ||
| return this.isAnimated ? millis : 0; |
There was a problem hiding this comment.
I think you need still need to respect data.animated.
@dr-itz do you think we can deprecate the animated property with this change? Was there a specific use case why this was introduced?
There was a problem hiding this comment.
It is respected in line 31
There was a problem hiding this comment.
Oh I don't really remember, but I guess this was for ngx-bootstrap compatibility. So yeah, having this here feels kinda pointless
|
|
3 participants
Uh oh!
There was an error while loading. Please reload this page.