Skip to content

Feature/shimmer presentation state #14

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Feature/shimmer presentation state #14

wants to merge 7 commits into from
+318 −168

Conversation

@BohdanNikoletti
Copy link
Contributor

@BohdanNikoletti BohdanNikoletti commented Sep 29, 2019

Start to review this pull request after adopting #13

@BohdanNikoletti BohdanNikoletti marked this pull request as ready for review October 3, 2019 11:25
@BohdanNikoletti BohdanNikoletti changed the base branch from master to revert-10-master October 3, 2019 11:28
@BohdanNikoletti BohdanNikoletti changed the base branch from revert-10-master to master October 3, 2019 11:28
@BohdanNikoletti
Copy link
Contributor Author

BohdanNikoletti commented Oct 3, 2019 •
edited
Loading

Idea behind this PR is adopt ShimmerState. With ShimmerState we can support multiple states of Shimmer Widget. For example in case of my App. I need to pause shimmer animation in some point. After completely return shimmer effect and simply draw child instead.
Important: I 've tested it manually. Double check pls.🙏🏻

BohdanNikoletti
BohdanNikoletti commented Oct 3, 2019
View reviewed changes
@BohdanNikoletti
Copy link
Contributor Author

BohdanNikoletti commented Oct 3, 2019

I think my solution can be combined with solution in. #12

@hnvn
Copy link
Owner

hnvn commented Oct 16, 2019

Sorry for late replay, I am too busy with other repositories, I will try to look at this feature at the weekend

@BohdanNikoletti BohdanNikoletti requested a review from hnvn February 10, 2020 12:57
@WilliamCunhaCardoso
Copy link

WilliamCunhaCardoso commented Aug 28, 2020

Please add tests considering each state of the shimmer and check with expect

@BohdanNikoletti
Copy link
Contributor Author

BohdanNikoletti commented Aug 28, 2020

Please add tests considering each state of the shimmer and check with expect

Ok. Going to add tests.

@BohdanNikoletti
Copy link
Contributor Author

BohdanNikoletti commented Aug 31, 2020

Please add tests considering each state of the shimmer and check with expect

Added some tests in latest PR. Could you take a look at what do you think?

@BohdanNikoletti
Copy link
Contributor Author

BohdanNikoletti commented Aug 31, 2020

Also, make some changes and introduce an animation controller provider. To make AnimationController testable.

@WilliamCunhaCardoso
Copy link

WilliamCunhaCardoso commented Aug 31, 2020

Please add tests considering each state of the shimmer and check with expect

Added some tests in latest PR. Could you take a look at what do you think?

Looks good to me. We must wait for @hnvn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hnvn hnvn Awaiting requested review from hnvn

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BohdanNikoletti @hnvn @WilliamCunhaCardoso