feat: improve pan interaction and animation state management #51
Conversation
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| @@ -85,6 +95,7 @@ export function setValues( | |||
| return setVariants(state, [definition]) | |||
| } | |||
| else { | |||
| setStateTarget(state, definition) | |||
| setTarget(state.visualElement, definition as any) | |||
There was a problem hiding this comment.
The use of definition as any in the setTarget function call is a type assertion that bypasses TypeScript's static type checking. This could lead to runtime errors if definition does not meet the expected structure required by setTarget. To improve type safety and maintainability, consider validating definition or refining its type definition to ensure it aligns with what setTarget expects.
| @@ -95,7 +106,7 @@ function setVariants(state: MotionState, variantLabels: string[]) { | |||
| reversedLabels.forEach((key) => { | |||
| const variant = visualElement.getVariant(key) | |||
| variant && setTarget(visualElement, variant) | |||
|
|
|||
| setStateTarget(state, variant) | |||
| if (visualElement.variantChildren) { | |||
| visualElement.variantChildren.forEach((child) => { | |||
| setVariants(mountedStates.get(child.current as HTMLElement), variantLabels) | |||
There was a problem hiding this comment.
The recursive call to setVariants within the forEach loop handling variantChildren could lead to performance issues or stack overflow errors, especially with a deep or cyclic structure of children. To prevent potential issues, consider implementing a non-recursive approach or adding safeguards such as depth limits or cycle detection.
| @@ -117,8 +117,8 @@ function createAnimationFactories( | |||
| ): AnimationFactory[] { | |||
| const factories: AnimationFactory[] = [] | |||
|
|
|||
| new Set(Object.keys(this.target)).forEach((key: any) => { | |||
| if (!hasChanged(this.visualElement.getValue(key), this.target[key])) | |||
| Object.keys(this.target).forEach((key: any) => { | |||
There was a problem hiding this comment.
The variable key is typed as any in the forEach loop, which undermines TypeScript's type safety. To enhance type safety and prevent potential runtime errors, it's recommended to specify a more precise type for key that aligns with the properties expected in this.target.
Suggested Change:
Object.keys(this.target).forEach((key: keyof typeof this.target) => {...})| new Set(Object.keys(this.target)).forEach((key: any) => { | ||
| if (!hasChanged(this.visualElement.getValue(key), this.target[key])) | ||
| Object.keys(this.target).forEach((key: any) => { | ||
| if (!hasChanged(prevTarget[key], this.target[key])) | ||
| return | ||
|
|
||
| this.baseTarget[key] ??= style.get(this.element, key) as string |
There was a problem hiding this comment.
The line assumes that style.get(this.element, key) will always return a string, which might not be the case, potentially leading to type inconsistencies or errors. It's safer to handle the possibility of different return types explicitly.
Suggested Change:
this.baseTarget[key] ??= style.get(this.element, key) as string | undefined
Save previous animation state values