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

Is _disableAnimation still used? #3395

Open
kirsty-hames opened this issue Apr 18, 2023 · 4 comments
Open

Is _disableAnimation still used? #3395

kirsty-hames opened this issue Apr 18, 2023 · 4 comments
Labels

Comments

@kirsty-hames
Copy link
Contributor

I was just reviewing the prefers-reduced-motion PR and noticed we had config for _disableAnimation in config.json but this doesn't seem to be referenced anywhere in Adapt. Is this now redundant config? Looking at the blame history it got added during an assets tidy so it may have been added in error.

@oliverfoster
Copy link
Member

oliverfoster commented Apr 18, 2023

https://github.com/search?q=org%3Aadaptlearning+_disableAnimation&type=code

There are two properties:
_disableAnimationFor an array of html class strings for matching
_disableAnimation to turn on and off absolutely

They control the drawer, notify, page load and scrollTo animations and add a disable-animation class to the html tag.

Schema definition:
adaptlearning/adapt-contrib-core@37fcbad

Core code:
https://github.com/adaptlearning/adapt-contrib-core/blob/b8cd90f6a914de8c31d8dab41501d791733d6d26/js/adapt.js#L178-L194

@kirsty-hames
Copy link
Contributor Author

https://github.com/search?q=org%3Aadaptlearning+_disableAnimation&type=code

There are two properties: _disableAnimationFor an array of html class strings for matching _disableAnimation to turn on and off absolutely

They control the drawer, notify, page load and scrollTo animations and add a disable-animation class to the html tag.

Schema definition: adaptlearning/adapt-contrib-core@37fcbad

Core code: https://github.com/adaptlearning/adapt-contrib-core/blob/b8cd90f6a914de8c31d8dab41501d791733d6d26/js/adapt.js#L178-L194

Thanks @oliverfoster. I must have had a typo in my search key!

If drawer, notify, page load and scrollTo animations count as non-essential motion, should these also be controlled by the _isPrefersReducedMotionEnabled config?

@oliverfoster
Copy link
Member

oliverfoster commented Apr 18, 2023

Mmmm. . Not entirely sure. Visua11y currently has a no animations... Which is independent of prefers-reduced-motion. Hmm... Probably yes.

Prefers reduced should probably trigger no animations and no animations probably should do prefers reduced motion. It's kind of one and the same behavior is it not?

Except that no animations is currently user configurable (with visua11y) and dev configurable (with the aforementioned properties) whereas prefers reduced motion is external to the browser / dev / visua11y config. So there's a question of precedence and configurability.

@kirsty-hames
Copy link
Contributor Author

Prefers reduced should probably trigger no animations and no animations probably should do prefers reduced motion. It's kind of one and the same behaviour is it not?

Yep I agree.

Except that no animations is currently user configurable (with visua11y) and dev configurable (with the aforementioned properties) whereas prefers reduced motion is external to the browser / dev / visua11y config. So there's a question of precedence and configurability.

I think user preference should always take precedence but with the various configs and use cases it does get a little confusing. In theory, any of the three configs should enable the same behaviour (remove any core UI or theme animations). I think the current implementation is fine as is but something we might be able to improve.

@oliverfoster oliverfoster added bug and removed question labels Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants