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

refactor(animation): refactor Animation class to a separate module #356

Closed
wants to merge 1 commit into from

Conversation

samavati
Copy link
Contributor

Pull Request Description

📜 Description

This pull request refactors the Animation class to improve code organization and maintainability. The Animation class has been moved from rive.ts to a new module under js/src/animation/. This change encapsulates the animation logic within its own module, making the codebase more modular and easier to navigate.

🛠️ Changes

  • New Module: Created a new file Animation.ts under js/src/animation/ to house the Animation class.
  • Export: Added an index.ts file in the js/src/animation/ directory to export the Animation class.
  • Imports Update: Updated the import statements in rive.ts to use the newly created Animation module.
  • Code Cleanup: Removed the Animation class definition from rive.ts.

💡 Motivation

The primary motivation for this refactor is to enhance code organization. By moving the Animation class to its own module, we achieve:

  • Better separation of concerns.
  • Improved readability and maintainability.
  • Easier testing and potential future enhancements to the Animation class.

🔍 Detailed Changes

js/src/animation/Animation.ts

  • Class Definition: The Animation class is now defined in this file.
  • Methods: Includes methods for managing animation state, advancing the animation, applying keyframe values, and cleaning up the animation instance.

js/src/animation/index.ts

  • Export: Exports the Animation class from Animation.ts.

js/src/rive.ts

  • Import Update: Imports the Animation class from the new js/src/animation/ module.
  • Code Removal: Removed the Animation class definition that was previously embedded in this file.

🧪 Testing

  • Unit Tests: Ensured that existing unit tests for the Animation class continue to pass.
  • Integration Tests: Verified that the Animation class works seamlessly with the rest of the codebase after the refactor.

📈 Impact

This refactor should have no functional impact on the existing features. It is purely an internal code organization improvement.

📝 Notes

  • Developers working on the Animation class should now refer to the new module under js/src/animation/.
  • Ensure that any future changes to the Animation class are made in the new Animation.ts file.

Thank you for reviewing this pull request. Looking forward to your feedback and approval!

- Moved the `Animation` class from `rive.ts` to a new file `Animation.ts` under `js/src/animation/`.
- Added a new `index.ts` file in the `js/src/animation/` directory to export the `Animation` class.
- Updated imports in `rive.ts` to use the newly created `Animation` module.

This refactor improves code organization by separating the `Animation` class into its own module, making the codebase more modular and easier to maintain.
@samavati
Copy link
Contributor Author

Hi @zplata,

I haven't heard back about this pull request yet and I'm eager to get your feedback on the refactoring changes to the Animation class.

Specifically, I'd love to know your thoughts on separating the Animation class into a separate module for better development.

Thanks for your time!
Ehsan

Copy link
Contributor

@zplata zplata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed description here! Great to see some movement on cleaning up the big rive.ts file.

I think this change looks safe to me, but will defer to @HayesGordon and @bodymovin on helping get this change in if they agree too!

@HayesGordon
Copy link
Contributor

Hi @samavati, we'll get this merged in. Thank you for your contribution and great description.

Just a note that I will need to co-author this with you. We'll generate a new commit from this PR to our upstream MONO repository, which will then be available in the public repo. Once that is out, I'll close this PR.

rive-engineering pushed a commit that referenced this pull request Jun 21, 2024
Brings in: #356

Also fixes warning when building:
```
Should not import the named export 'version' (imported as 'packageData') from default-exporting module (only default export is available soon)
```

Diffs=
6f8c518ff chore: community contribution wasm refactor nested module (#7457)

Co-authored-by: Ehsan Samavati <[email protected]>
Co-authored-by: Gordon <[email protected]>
@HayesGordon
Copy link
Contributor

This has been merged as part of: fa1319e

Thanks again for the contribution.

@samavati
Copy link
Contributor Author

This has been merged as part of: fa1319e

Thanks again for the contribution.

@HayesGordon Thank you
I'm planning to do more refactors in this repo and also contribute to develop new features
for example, the next one would be #358
I'm so glad that I can help here and make the developers lives easier ☺️
Thank you for this great project

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.

3 participants