-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[media-library] add options for getAssetInfoAsync #9405
[media-library] add options for getAssetInfoAsync #9405
Conversation
Sorry @tsapeta @lukmccall, this PR has been left open for quite sometimes 😅 , I wasn't been able to find time until now. |
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.
Please add an entry to the changelog :D
packages/expo-media-library/ios/EXMediaLibrary/EXMediaLibrary.m
Outdated
Show resolved
Hide resolved
Done |
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 💪 Thanks for contributing 🎉🎉🎉 Awesome work 🏅
There is still one thing left which is the bare-expo app. I wonder how should we update the demo screen for this API to reflect the possibility of changing the options? Right now, the screen just call |
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.
Looks promising, thanks 👍 Just a few suggestions.
Co-authored-by: Tomasz Sapeta <1714764+tsapeta@users.noreply.github.com>
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.
Perfect, thank you for fixing my suggestions! 🙂
One more thing — looks like there are two conflicts with master, could you resolve them?
Done 👍 |
packages/expo-media-library/ios/EXMediaLibrary/EXMediaLibrary.m
Outdated
Show resolved
Hide resolved
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@student.uj.edu.pl>
@jarvisluong I've just published |
Awesome! |
Why
This is an enhanced PR from #8800
How
I added
options
as the optional second parameter toMediaLibrary.getAssetInfoAsync
.Test Plan
Currently I modified
MediaDetailsScreen
in bare-expo app with payload ofshouldDownloadFromNetwork
tofalse
so that the response payload also contains the fieldisNetworkAsset
.Open question
Since this changes the public API of
MediaLibrary
, I wonder how should I update the bare-expo app to reflect this option?