-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[image_picker] Update README for image_picker's retrieveLostData() to support multiple files #4336
Conversation
Hi @reez12g, thank you for the PR. Please make sure you update the CHANGELOG.md and pubspec.yaml files. Since the README is mostly read on pub.dev, updating the README.md is therefore considered a change that needs to be published and requires the patch number to be updated. Also I feel the example code shown in the README is confusing and could be written in a better way (see comment). |
if (response.file != null) { | ||
setState(() { | ||
if (response.type == RetrieveType.video) { | ||
_handleVideo(response.file); | ||
} else { | ||
_handleImage(response.file); | ||
} | ||
}); | ||
if (response.files.length >= 2 ) { | ||
_imageFileList = response.files | ||
} else { | ||
setState(() { | ||
if (response.type == RetrieveType.video) { | ||
_handleVideo(response.file); | ||
} else { | ||
_handleImage(response.file); | ||
} | ||
}}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example looks a bit confusing, as when dealing with multiple files it simply stores the list in the _imageFileList
variable, while when dealing with a single file it is calling the _handleVideo()
or _handleImage()
method and updating the state (as per setState
call). Seems these situations are handles very differently. Maybe you can update the code to a more simple example:
if (response.files != null) {
for(final XFile file in response.files) {
_handleFile(file);
}
} else {
_handleError(response.exception);
}
The above example simply shows how to handle lost data and leaves it to the implementing users to decide how to handle the files that are retrieved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvanbeusekom
Thank you so much for your very thoughtful comments!
The example you wrote was very clear.
I've taken it in as a commit!
Thanks for the comment! I'll check it out! |
I updated the CHANGELOG.md and pubspec.yaml. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@reez12g could you maybe rebase your PR on the |
… support multiple files
It's done! Thank you very much. However, the |
@mvanbeusekom |
Thank you for rebasing @reez12g, looking good now. The This is nothing related to your PR, I will add the |
…ata() to support multiple files (flutter/plugins#4336)
I understand now! |
This was merged before a fix for our CI landed, so even though the change has been correctly pushed to pub.dev, a tag for this failed to publish into the repo. This is not a big deal, but I just wanted to let you know why this is red in The fix for CI has landed (right after this commit), so from now on the Apologies for the inconvenience! |
I understand! Thank you for taking the time to tell me! |
Thanks again for the fix, keep them coming! 🙏 |
…ata() to support multiple files (flutter/plugins#4336)
… support multiple files (flutter#4336)
… support multiple files (flutter#4336)
issues fixed by this PR
Pre-launch Checklist
dart format
.)[shared_preferences]
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.