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

Animate AI Assistant button and panel #1097

Closed
wants to merge 11 commits into from
Closed

Animate AI Assistant button and panel #1097

wants to merge 11 commits into from

Conversation

lukemelia
Copy link
Contributor

@lukemelia lukemelia commented Mar 18, 2024

Screen.Recording.2024-03-20.at.4.03.35.PM.mov

@lukemelia lukemelia marked this pull request as draft March 18, 2024 03:30
@lukemelia lukemelia requested review from ef4 and nickschot March 18, 2024 03:30
Copy link

github-actions bot commented Mar 18, 2024

Test Results

    1 files  ±0      1 suites  ±0   8m 4s ⏱️ +24s
535 tests ±0  406 ✔️  - 125  4 💤 ±0      0 ±    0  125 🔥 +125 
535 runs  ±0  281 ✔️  - 250  4 💤 ±0  125 +125  125 🔥 +125 

For more details on these errors, see this check.

Results for commit df61a89. ± Comparison against base commit f76bbd3.

♻️ This comment has been updated with latest results.

@lukemelia lukemelia force-pushed the use-motion branch 2 times, most recently from f836a10 to cd5b687 Compare March 18, 2024 18:47
@nickschot
Copy link
Contributor

nickschot commented Mar 19, 2024

https://www.loom.com/share/cd5d9bdc3e194ecba4b6a1fb83bc1e6b

I remember that this was still something we have to finalise after finishing the implementation of the scale correction/transform based animating. I don't think the current version has a fill: forwards style thing going on by default (if I remember correctly), so it's likely related to sprite.lockStyles which locks position and width with inline CSS. Apparently that isn't currently automatically cleaned up after an animation, though we do have an unlockStyles function as well. This is called within the transitionRunner. You could see if calling it after the animation promise resolves works (for now?). I don't remember if it may have unexpected side effects at this point.

edit: to add to that, I think with the current timeline functionality (animating every sprite for the entire duration instead of just when it's animated), anchoring, padding etc. we may be able to get rid of lockStyles by integrating it into the keyframes.

@lukemelia
Copy link
Contributor Author

Thanks @nickschot. I updated this PR to use a StaticBehavior to fix the width during the animation. I generalized the fill behavior so that you can opt StaticBehavior into forward filling like the other behaviors (except Wait) already did by default.

Integrating the lockStyles into the timeline is an interesting idea that likely simplifies the overall code and mental model.

@lukemelia
Copy link
Contributor Author

lukemelia commented Mar 20, 2024

After discussion with the team and with Chris, we decided to defer animation work on host, so I'll close this PR after extracting the non-animation changes from it into new PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants