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.

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

Merged

Conversation

jarvisluong
Copy link
Contributor

Why

This is an enhanced PR from #8800

How

I added options as the optional second parameter to MediaLibrary.getAssetInfoAsync.

Test Plan

Currently I modified MediaDetailsScreen in bare-expo app with payload of shouldDownloadFromNetwork to false so that the response payload also contains the field isNetworkAsset.

Open question

Since this changes the public API of MediaLibrary, I wonder how should I update the bare-expo app to reflect this option?

@jarvisluong jarvisluong marked this pull request as ready for review July 26, 2020 16:09
@jarvisluong
Copy link
Contributor Author

Sorry @tsapeta @lukmccall, this PR has been left open for quite sometimes 😅 , I wasn't been able to find time until now.

Copy link
Contributor

@lukmccall lukmccall left a 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/src/MediaLibrary.ts Outdated Show resolved Hide resolved
@jarvisluong
Copy link
Contributor Author

Please add an entry to the changelog :D

Done

Copy link
Contributor

@lukmccall lukmccall left a 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 🏅

@jarvisluong
Copy link
Contributor Author

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 getAssetInfoAsync directly.

Copy link
Member

@tsapeta tsapeta left a 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.

docs/pages/versions/unversioned/sdk/media-library.md Outdated Show resolved Hide resolved
packages/expo-media-library/src/MediaLibrary.ts Outdated Show resolved Hide resolved
packages/expo-media-library/src/MediaLibrary.ts Outdated Show resolved Hide resolved
packages/expo-media-library/src/MediaLibrary.ts Outdated Show resolved Hide resolved
packages/expo-media-library/src/MediaLibrary.ts Outdated Show resolved Hide resolved
packages/expo-media-library/src/MediaLibrary.ts Outdated Show resolved Hide resolved
docs/pages/versions/unversioned/sdk/media-library.md Outdated Show resolved Hide resolved
Jarvis Luong and others added 2 commits July 28, 2020 21:56
Co-authored-by: Tomasz Sapeta <1714764+tsapeta@users.noreply.github.com>
@jarvisluong jarvisluong requested a review from tsapeta July 28, 2020 18:59
Copy link
Member

@tsapeta tsapeta left a 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?

@jarvisluong
Copy link
Contributor Author

Perfect, thank you for fixing my suggestions! 🙂
One more thing — looks like there are two conflicts with master, could you resolve them?

Done 👍

@jarvisluong jarvisluong requested a review from tsapeta July 29, 2020 08:40
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@student.uj.edu.pl>
@lukmccall lukmccall merged commit b326bbe into expo:master Jul 29, 2020
@jarvisluong jarvisluong deleted the feature/media-library-advanced-options branch July 29, 2020 12:39
@tsapeta
Copy link
Member

tsapeta commented Jul 29, 2020

@jarvisluong I've just published expo-media-library@8.5.0 that includes this change 😉

@jarvisluong
Copy link
Contributor Author

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants