-
-
Notifications
You must be signed in to change notification settings - Fork 304
feat: allow using enterDuration and exitDurationOfPrev for figure and bg enter/exit animation config
#806
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
base: dev
Are you sure you want to change the base?
Conversation
… bg enter/exit animation config
Summary of ChangesHello @MakinoharaShoko, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the animation system by providing developers with granular control over the duration of enter and exit animations for backgrounds and figures. It achieves this by refactoring the animation configuration storage, moving it from a temporary manager to a centralized Redux store. This change not only offers more flexibility in customizing visual transitions but also establishes a more robust and manageable state for animation settings across the application. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the animation configuration to use the Redux store, which is a good move for state management. It also adds enterDuration and exitDuration to provide more control over animations. The implementation is mostly solid, but I've identified a few areas for improvement. There are some typos and redundant code that can be cleaned up. More importantly, there's a potential bug in how a duration of 0 is handled, which could prevent instantaneous animations from working as expected. My review includes suggestions to fix these issues.
| if (enterDuration) { | ||
| webgalStore.dispatch( | ||
| stageActions.updateAnimationSettings({ target: 'bg-main', key: 'enterDuration', value: enterDuration }), | ||
| ); | ||
| } | ||
| if (exitDuration) { | ||
| webgalStore.dispatch( | ||
| stageActions.updateAnimationSettings({ target: 'bg-main', key: 'exitDuration', value: exitDuration }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional checks if (enterDuration) and if (exitDuration) are problematic. If a duration of 0 is passed, which is a valid value for an instant transition, the condition will evaluate to false, and the setting will not be updated in the store. You should check for null or undefined instead to correctly handle a duration of 0.
| if (enterDuration) { | |
| webgalStore.dispatch( | |
| stageActions.updateAnimationSettings({ target: 'bg-main', key: 'enterDuration', value: enterDuration }), | |
| ); | |
| } | |
| if (exitDuration) { | |
| webgalStore.dispatch( | |
| stageActions.updateAnimationSettings({ target: 'bg-main', key: 'exitDuration', value: exitDuration }), | |
| ); | |
| } | |
| if (getNumberArgByKey(sentence, 'enterDuration') != null) { | |
| webgalStore.dispatch( | |
| stageActions.updateAnimationSettings({ target: 'bg-main', key: 'enterDuration', value: enterDuration }), | |
| ); | |
| } | |
| if (exitDuration != null) { | |
| webgalStore.dispatch( | |
| stageActions.updateAnimationSettings({ target: 'bg-main', key: 'exitDuration', value: exitDuration }), | |
| ); | |
| } |
| if (enterDuration) { | ||
| webgalStore.dispatch( | ||
| stageActions.updateAnimationSettings({ target: key, key: 'enterDuration', value: enterDuration }), | ||
| ); | ||
| } | ||
| if (exitDuration) { | ||
| webgalStore.dispatch( | ||
| stageActions.updateAnimationSettings({ target: key, key: 'exitDuration', value: exitDuration }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional checks if (enterDuration) and if (exitDuration) will fail if a duration of 0 is provided. A duration of 0 is a valid value for an instantaneous animation, but if (0) evaluates to false. To fix this bug, you should explicitly check against null and undefined.
| if (enterDuration) { | |
| webgalStore.dispatch( | |
| stageActions.updateAnimationSettings({ target: key, key: 'enterDuration', value: enterDuration }), | |
| ); | |
| } | |
| if (exitDuration) { | |
| webgalStore.dispatch( | |
| stageActions.updateAnimationSettings({ target: key, key: 'exitDuration', value: exitDuration }), | |
| ); | |
| } | |
| if (getNumberArgByKey(sentence, 'enterDuration') != null) { | |
| webgalStore.dispatch( | |
| stageActions.updateAnimationSettings({ target: key, key: 'enterDuration', value: enterDuration }), | |
| ); | |
| } | |
| if (exitDuration != null) { | |
| webgalStore.dispatch( | |
| stageActions.updateAnimationSettings({ target: key, key: 'exitDuration', value: exitDuration }), | |
| ); | |
| } |
| duration = | ||
| webgalStore.getState().stage.animationSettings.find((setting) => setting.target === target)?.enterDuration ?? | ||
| duration; | ||
| // 走默认动画 | ||
| let animation: IAnimationObject | null = generateUniversalSoftInAnimationObj(realTarget ?? target, duration); | ||
|
|
||
| const transformState = webgalStore.getState().stage.effects; | ||
| const targetEffect = transformState.find((effect) => effect.target === target); | ||
|
|
||
| const animarionName = WebGAL.animationManager.nextEnterAnimationName.get(target); | ||
| const animarionName = webgalStore | ||
| .getState() | ||
| .stage.animationSettings.find((setting) => setting.target === target)?.enterAnimationName; | ||
| if (animarionName && !targetEffect) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in the variable name animarionName (should be animationName). Additionally, the code queries the animationSettings from the store twice. You can optimize this by performing the lookup once and reusing the result.
const animationSetting = webgalStore
.getState()
.stage.animationSettings.find((setting) => setting.target === target);
duration = animationSetting?.enterDuration ?? duration;
// 走默认动画
let animation: IAnimationObject | null = generateUniversalSoftInAnimationObj(realTarget ?? target, duration);
const transformState = webgalStore.getState().stage.effects;
const targetEffect = transformState.find((effect) => effect.target === target);
const animationName = animationSetting?.enterAnimationName;
if (animationName && !targetEffect) {| duration = | ||
| webgalStore.getState().stage.animationSettings.find((setting) => setting.target + '-off' === target) | ||
| ?.exitDuration ?? duration; | ||
| // 走默认动画 | ||
| let animation: IAnimationObject | null = generateUniversalSoftOffAnimationObj(realTarget ?? target, duration); | ||
| const animarionName = WebGAL.animationManager.nextExitAnimationName.get(target); | ||
| const animarionName = webgalStore | ||
| .getState() | ||
| .stage.animationSettings.find((setting) => setting.target + '-off' === target)?.exitAnimationName; | ||
| if (animarionName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the 'enter' case, there's a typo in animarionName and a redundant lookup for animationSettings. You can optimize this by finding the setting once and reusing it.
const animationSetting = webgalStore
.getState()
.stage.animationSettings.find((setting) => setting.target + '-off' === target);
duration = animationSetting?.exitDuration ?? duration;
// 走默认动画
let animation: IAnimationObject | null = generateUniversalSoftOffAnimationObj(realTarget ?? target, duration);
const animationName = animationSetting?.exitAnimationName;
if (animationName) {
Deploying webgal-dev with
|
| Latest commit: |
9e72da2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a9cf3044.webgal-dev.pages.dev |
| Branch Preview URL: | https://stage-obj-duration.webgal-dev.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
测试案例全部通过, 没有什么问题
有一些问题, 待解决
| let duration = getNumberArgByKey(sentence, 'duration') ?? 1000; | ||
| const enterDuration = getNumberArgByKey(sentence, 'enterDuration') ?? duration; | ||
| duration = enterDuration; | ||
| const exitDuration = getNumberArgByKey(sentence, 'exitDurationOfPrev'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
其实既然退场动画叫 exit 而不是 exitOfPrev 的话, 那其实这里似乎也没有什么必要加后缀, 短一点也比较好打, 在文档里或者编辑器里说明一下就好了, changeFigure那边同理
HardyNLee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已解决 #701 , 并通过了所有测试案例
No description provided.