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

Radical refactoring. #157

Merged
merged 2 commits into from
Nov 28, 2020
Merged

Conversation

awhitford
Copy link
Collaborator

Most Animations duplicated a lot of code. This refactoring introduces common base classes so that each custom animation really just needs to focus on the critical things like initialization Animations, and then building the widgets based on Animation Values.

Overall, the result is much more flexible because since logic was split between AnimatedText and AnimatedTextKit, one can now easily mix and match different Animations. It is also much easier to introduce new custom text animations.

@awhitford
Copy link
Collaborator Author

@aagarwal1012 This refactoring may be worthy of a Version 3.

I created backward compatibility wrappers for the existing animations so that users would be minimally impacted. However, I wonder if the docs should be updated to show people a different way to build animations.

For example, instead of:

  TypewriterAnimatedTextKit(
    onTap: () {
        print("Tap Event");
      },
    text: [
      "Alpha",
    ],
    textStyle: TextStyle(
        fontSize: 30.0,
        fontFamily: "Agne"
    ),
  ),

One could say:

  AnimatedTextKit(
    onTap: () {
        print("Tap Event");
      },
    animatedTexts: [
       TypewriterAnimatedText("Alpha",
         textStyle: TextStyle(
           fontSize: 30.0,
           fontFamily: "Agne"
         ),
       ),
    ],
  ),

The advantage of the latter is that animatedTexts no longer need to be just one type, so you can animate between different animations -- Fade, Scale, Typer, etc.

@aagarwal1012
Copy link
Owner

aagarwal1012 commented Nov 27, 2020

@awhitford, I totally agree with your point -- I had somewhere in my mind of doing similar to this PR, but before releasing the v3.0.0 proper documentation and testing will require.

@awhitford
Copy link
Collaborator Author

I should also point out that this PR creates more consistency across Animations. For example, not all animations supported the same flags like repeatForever and totalRepeatCount, but now since they extend the same base class, each different animation has the same functionality. (Except LiquidFillText because the internals of that differed significantly, so I have left that one alone for now.)

I suggest cutting a 2.6.0 release before merging this commit. Those changes are minor, sound, and serve a solid rollback foundation in case anything does pop up unexpectedly.

To address testing concerns, perhaps cutting a 3.0-pre-release may make sense and then it can age in the wild for a little.

The README documentation is still correct. As far I know, I intentionally did not break any interface. I even adjusted a couple of animations to default to repeatForever: true because that matched existing behavior.

Perhaps I can review the DartDoc to ensure that the package score is not adversely impacted. I tried to be mindful of adding docs to public things -- and actually the less code ultimately makes documentation easier.

I expected to see the CodeCov results, but they aren't appearing on this PR. I was hoping to verify that code coverage did not decrease. Do you know why that is missing?

@aagarwal1012
Copy link
Owner

Travis CI is not running, that's why Codecov is not showing in this PR.

Copy link
Owner

@aagarwal1012 aagarwal1012 left a comment

Choose a reason for hiding this comment

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

@awhitford, your changes look good to me. 💯

@aagarwal1012 aagarwal1012 merged commit ceeefea into aagarwal1012:master Nov 28, 2020
@awhitford
Copy link
Collaborator Author

CodeCov is now reporting 91%! 🎉 So it looks like this resolves #128 too!

@awhitford awhitford deleted the radical_refactor branch December 7, 2020 22:26
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