Fix/hover variants #34
Conversation
…for child activation
- Updated tsconfig.json files to exclude only node_modules and include .tsx files in motion package. - Modified vite.config.ts to refine the exclusion patterns for build artifacts. - Added gesture test case for Motion component to verify hover effects.
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| // @ts-ignore | ||
| <Motion hover="hover" data-testid="motion"> | ||
| {/* @ts-ignore */} |
There was a problem hiding this comment.
The use of @ts-ignore to bypass TypeScript's static type checking can lead to unnoticed type errors and reduce the code's maintainability. It's recommended to address the underlying type issues rather than using @ts-ignore. This might involve updating type definitions or adjusting the usage to align with expected types.
| await delay(300) | ||
| expect(wrapper.find('[data-testid="motion-child"]').element.getAttribute('style')).toBe('transform: scale(1.2);') | ||
| wrapper.find('[data-testid="motion"]').trigger('pointerleave') | ||
| await delay(300) |
There was a problem hiding this comment.
The test relies on hardcoded delays (await delay(300)) to account for animations, which can lead to flaky tests. Instead, consider using more reliable synchronization mechanisms like waiting for specific changes in the DOM or using animation hooks provided by the framework to ensure the test's reliability and robustness.
| @@ -201,7 +201,7 @@ export class MotionState { | |||
| return | |||
| this.activeStates[name] = isActive | |||
| this.visualElement.variantChildren?.forEach((child) => { | |||
| ((child as any).state as MotionState).setActive(name, isActive, false) | |||
| ((child as any).state as MotionState).setActive(name, isActive, !isActive) | |||
| }) | |||
There was a problem hiding this comment.
The recursive call to setActive inverts the isActive flag, which could lead to unintended toggling of state in child elements. This might cause inconsistent behavior in animations or state transitions.
Suggested Fix:
Ensure the isActive flag is passed correctly to child elements without inversion unless specifically required by the design:
child.setActive(name, isActive, isAnimate)| @@ -12,7 +12,7 @@ export default defineConfig({ | |||
| dts({ | |||
| cleanVueFileName: true, | |||
| outDir: 'dist', | |||
| exclude: ['src/test/**', 'src/**/story/**', 'src/**/*.story.vue'], | |||
| exclude: ['src/**/__tests__/**', 'src/**/story/**', 'src/**/*.story.vue'], | |||
| afterBuild: async () => { | |||
| // pnpm build:plugins | |||
| execSync('pnpm build:plugins', { stdio: 'inherit', cwd: path.resolve(__dirname, '../plugins') }) | |||
There was a problem hiding this comment.
Using execSync to execute shell commands can lead to potential security risks if not properly sanitized and can block the Node.js event loop, affecting performance. Consider using the asynchronous exec function to avoid blocking and add error handling to manage any failures gracefully.
Suggested Change:
import { exec } from 'node:child_process';
// Replace execSync with exec
exec('pnpm build:plugins', { stdio: 'inherit', cwd: path.resolve(__dirname, '../plugins') }, (error, stdout, stderr) => {
if (error) {
console.error('Error executing build:plugins:', stderr);
throw error;
}
console.log(stdout);
});| @@ -12,7 +12,7 @@ export default defineConfig({ | |||
| dts({ | |||
| cleanVueFileName: true, | |||
| outDir: 'dist', | |||
| exclude: ['src/test/**', 'src/**/story/**', 'src/**/*.story.vue'], | |||
| exclude: ['src/**/__tests__/**', 'src/**/story/**', 'src/**/*.story.vue'], | |||
| afterBuild: async () => { | |||
| // pnpm build:plugins | |||
| execSync('pnpm build:plugins', { stdio: 'inherit', cwd: path.resolve(__dirname, '../plugins') }) | |||
There was a problem hiding this comment.
The current implementation does not handle errors that may occur during the execution of the pnpm build:plugins command. This can cause the build process to fail without adequate logging or error handling, making debugging more difficult.
Recommendation:
Implement error handling for the command execution to ensure that any issues are properly logged and managed. This could involve checking the exit status of the command and logging any errors encountered during its execution.
fix #31
fix #32