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

Refactored code to move default logic from the State class to the StatefulWidget #131

Merged
merged 1 commit into from
Oct 23, 2020

Conversation

awhitford
Copy link
Collaborator

  • Moved _duration and _pause defaults to the StatefulWidget.
  • Added assertions to ensure proper usage.
  • Removed unused textWidgetList variables.
  • Reordered imports.
  • Tweaked indention.
  • Declared _texts final.
  • Added const keywords.

@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #131 into master will decrease coverage by 4.64%.
The diff coverage is 47.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
- Coverage   68.86%   64.22%   -4.65%     
==========================================
  Files           8        8              
  Lines         713      763      +50     
==========================================
- Hits          491      490       -1     
- Misses        222      273      +51     
Impacted Files Coverage Δ
lib/src/typewriter.dart 50.00% <21.42%> (-4.91%) ⬇️
lib/src/typer.dart 56.47% <27.27%> (-6.83%) ⬇️
lib/src/wavy.dart 64.46% <40.00%> (-3.06%) ⬇️
lib/src/fade.dart 56.66% <50.00%> (-6.63%) ⬇️
lib/src/scale.dart 59.59% <57.14%> (-7.08%) ⬇️
lib/src/rotate.dart 61.60% <57.89%> (-4.73%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4c3346...568ce66. Read the comment docs.

Comment on lines +136 to +139
_texts.add({
'text': text,
'pause': widget.pause,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idk why do we need this anyways pause is constant..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

YES! You're right! The refactoring helps make this crystal clear.

I'll go one step further... Why should _texts exist? It should simply reference widget.text.

I did not remove it because I wondered if there was a grand agenda to its existence. (🤔 I often wonder, "What was the original author thinking?")

I'm happy to reconsider removing it, but I would prefer doing it as a follow-up PR because it will impact other classes and the recent emoji-related changes may need to be tweaked. (I like to avoid too many ideas in a PR because it makes it more challenging to understand what was changed and why.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yaa I also felt that but I haven't included it WavyAnimatedTexKit, maybe it was a thing that all the authors that it might have some use can but never had one 🤔 @aagarwal1012 do you have any idea?

Copy link
Collaborator

@SirusCodes SirusCodes left a comment

Choose a reason for hiding this comment

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

I have seen the code but not ran it(trusting @awhitford with it)

@awhitford awhitford merged commit 26a81ef into aagarwal1012:master Oct 23, 2020
@awhitford awhitford deleted the defaults branch October 23, 2020 10:03
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