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

[config][animationMode]: setAnimationMode() does not work #8964

Closed
1 task done
dreynol opened this issue May 15, 2024 · 2 comments · Fixed by #8965
Closed
1 task done

[config][animationMode]: setAnimationMode() does not work #8964

dreynol opened this issue May 15, 2024 · 2 comments · Fixed by #8965
Labels
bug This issue is a bug in the code High Prio TOPIC Core

Comments

@dreynol
Copy link
Contributor

dreynol commented May 15, 2024

Bug Description

It is impossible to use setAnimationMode() to set the current animation to a valid mode.

Affected Component

Any component with animations (ui5-progress-indicator, ui5-panel, ui5-carousel, etc.)

Expected Behaviour

It is expected that calling setAnimationMode(mode), where mode is either a valid member of the AnimationMode enum/type (AnimationMode.Full, AnimationMode.Basic, AnimationMode.Minimal, or AnimationMode.None) or the corresponding string value ("full", "basic", "minimal", or "none"), would result in the current animation mode being changed to that value. This in turn is expected to modify the animation behavior of components that use animations. Specifically, setting the mode to "none" is expected to disable animations. This, however, is not the case.

// by default animationMode is initialized to "full"

setAnimationMode(AnimationMode.None);
console.log(getAnimationMode());
// expected output: "none"
// actual output: "full"

setAnimationMode("none"):
console.log(getAnimationMode());
// expected output: "none"
// actual output: "full"

setAnimationMode("None");
console.log(getAnimationMode());
// expected output: "full" (since the string "None" is not a valid type or value of AnimationMode)
// actual output: "None"

In the last example, while setAnimationMode("None") does change the currently set animation mode, it sets it to an invalid value so animations will not be disabled.

Isolated Example

https://stackblitz.com/edit/js-32lj5a?file=index.js,index.html

Steps to Reproduce

Try to use setAnimationMode(AnimationMode.None) or setAnimationMode("none") to disable animations for a component (e.g. a ui5-progress-indicator) and observe that animations still occur when the state of the component changes. (See "Expected Behaviour" above or the stackblitz link for more detail.)

Log Output, Stack Trace or Screenshots

No response

Priority

High

UI5 Web Components Version

1.24.3

Browser

Chrome

Operating System

No response

Additional Context

No response

Organization

No response

Declaration

  • I’m not disclosing any internal or sensitive information.
@dreynol dreynol added the bug This issue is a bug in the code label May 15, 2024
@dreynol
Copy link
Contributor Author

dreynol commented May 15, 2024

In this code from packages/base/src/config/AnimationMode.ts

const setAnimationMode = (animationMode: `${AnimationMode}`) => {
    if (animationMode in AnimationMode) {
        curAnimationMode = animationMode;
    }
};

the "in" operator in the if statement checks if the passed-in animationMode is a property in the AnimationMode enum, not if it is a value in the enum, as seems to be intended. So given the following enum,

enum AnimationMode {
    Full = "full",
    Basic = "basic",
    Minimal = "minimal",
    None = "none"
}

the only values of animationMode that will make the if-statement evaluate to true are "Full", "Basic", "Minimal", or "None" (capitalized as the AnimationMode keys are), which means setAnimationMode() can only be used to set curAnimationMode to one of those capitalized values. So setAnimationMode("None") will result in curAnimationMode = "None", but when determining whether to use animation, components like the ui5-progress-indicator compare the currently set mode to the lowercase value:

// packages/main/src/ProgressIndicator.ts
class ProgressIndicator extends UI5Element {
    [...]

    get shouldAnimate() {
        return getAnimationMode() !== AnimationMode.None;
    }
    
    [...]
}

That comparison will be "None" !== "none". So no matter what is passed to setAnimationMode(), the shouldAnimate() method will always return true.

dreynol added a commit to dreynol/ui5-webcomponents that referenced this issue May 15, 2024
Compare the value of the passed in animationMode to the values of the
AnimationMode enum (lowercase) rather than to the property (key) names
(uppercase). This will make it so that explicitly passing in a mode
(e.g. AnimationMode.None) or passing in the string value corresponding
to a mode (e.g. "none") will correctly result in the current mode being
set to the lowercase string value, which will make it possible to use
setAnimationMode() to disable component animations as intended.

Fixes SAP#8964
@GerganaKremenska GerganaKremenska self-assigned this May 15, 2024
@GerganaKremenska GerganaKremenska added this to New in Maintenance - Topic Core via automation May 15, 2024
@GerganaKremenska GerganaKremenska removed their assignment May 15, 2024
@GerganaKremenska
Copy link
Member

Hello colleagues,

Can you take over the issue regarding setAnimationMode(), it seems there is an issue with it.
It is reproducible in the stackblitz.

Best Regards,
Gergana

vladitasev pushed a commit that referenced this issue May 15, 2024
Compare the value of the passed in animationMode to the values of the
AnimationMode enum (lowercase) rather than to the property (key) names
(uppercase). This will make it so that explicitly passing in a mode
(e.g. AnimationMode.None) or passing in the string value corresponding
to a mode (e.g. "none") will correctly result in the current mode being
set to the lowercase string value, which will make it possible to use
setAnimationMode() to disable component animations as intended.

Fixes #8964
Maintenance - Topic Core automation moved this from New to Completed May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug in the code High Prio TOPIC Core
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

2 participants