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.

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

Feature/shimmer presentation state #14

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

Conversation

BohdanNikoletti
Copy link
Contributor

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
lib/shimmer.dart Outdated Show resolved Hide resolved
@BohdanNikoletti
Copy link
Contributor Author

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

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

@BohdanNikoletti
Copy link
Contributor Author

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

Ok. Going to add tests.

@BohdanNikoletti
Copy link
Contributor Author

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

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

@WilliamCunhaCardoso
Copy link

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