Add loading support to Image#33369
Merged
tvolkert merged 2 commits intoflutter:masterfrom May 29, 2019
tvolkert:image_progress
Merged
Add loading support to Image#33369tvolkert merged 2 commits intoflutter:masterfrom tvolkert:image_progress
tvolkert merged 2 commits intoflutter:masterfrom
tvolkert:image_progress
Conversation
9 tasks
Hixie
reviewed
May 27, 2019
Hixie
reviewed
May 27, 2019
Contributor
There was a problem hiding this comment.
should we also give the total number of frames then?
Contributor
Author
There was a problem hiding this comment.
We won't know that ahead of time.
Contributor
Author
There was a problem hiding this comment.
What's the use case you're considering?
Contributor
There was a problem hiding this comment.
a media progress bar that shows where you are in a looping animated GIF (like the youtube player progress bar)
Hixie
reviewed
May 27, 2019
Hixie
reviewed
May 27, 2019
Hixie
reviewed
May 27, 2019
Hixie
reviewed
May 27, 2019
Hixie
reviewed
May 27, 2019
Contributor
Author
|
PTAL |
goderbauer
reviewed
May 28, 2019
goderbauer
added
the
framework
Contributor
Author
|
PTAL |
goderbauer
reviewed
May 28, 2019
goderbauer
reviewed
May 28, 2019
Member
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
The actual implementation LGTM. However, the modification of the template seems odd to me - especially since the modified template isn't even used in this PR?
This adds two new builders to the `Image` class: * `frameBuilder`, which allows callers to control the widget created by an [Image]. * `loadingBuilder`, which allows callers fine-grained control over how to display loading progress of an image to the user. `FadeInImage` can be simplified by migrating to the new API. This is done in a follow-on commit. #32374
Contributor
|
|
Contributor
Author
|
FYI @renefloor in case you find this relevant for cached_network_image |
tvolkert
added a commit
that referenced
this pull request
May 31, 2019
This updates FadeInImage to use the new Image.frameBuilder API (added in #33369), to greatly simplify the implementation of FadeInImage. This also removes the FadeInImage.placeholderSemanticLabel property. This property was added in #28799 for the sake of completeness (the bug it fixed was the lack of any semantic label support in FadeInImage), but a placeholder is a transient visual artifact, not something that affects the underlying semantic meaning of the image.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This adds two new builders to the
Imageclass:frameBuilder, which allows callers to control the widgetcreated by an [Image].
loadingBuilder, which allows callers fine-grained controlover how to display loading progress of an image to the user.
FadeInImagecan be simplified by migrating to the new API.This is done in follow-on commit.
Related Issues
#32374
Tests
I added full test coverage of the two new properties (verified in merged
lcov.info). Specifically:Checklist
///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?