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

[ios][store-review] Refactor isSupported to isAvailableSync #6195

Merged
merged 3 commits into from
Nov 14, 2019
Merged

[ios][store-review] Refactor isSupported to isAvailableSync #6195

merged 3 commits into from
Nov 14, 2019

Conversation

danibonilha
Copy link
Contributor

Why

This PR refactors isSupported method from StoreReview module to isAvailableSync
#6129 issue

How

I've modified native code, package store-review and native-components to use isAvailableSync

Test Plan

I wrote a new test suite to validate this change

simulator
image
image

@danibonilha danibonilha changed the title Refactor/store review async support [ios][store-review] Refactor isSupported to isSupported Nov 6, 2019
@danibonilha danibonilha changed the title [ios][store-review] Refactor isSupported to isSupported [ios][store-review] Refactor isSupported to isAvailableSync Nov 6, 2019
Copy link
Member

@brentvatne brentvatne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small change requested, otherwise looks great, thank you!

@brentvatne brentvatne requested a review from EvanBacon November 6, 2019 23:47
Copy link
Contributor

@EvanBacon EvanBacon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add an entry to the CHANGELOG :)

@EvanBacon
Copy link
Contributor

Also great work on this the gifs are really appreciated!

@danibonilha
Copy link
Contributor Author

Updated Screenshots

image

@EvanBacon EvanBacon self-assigned this Nov 11, 2019
Copy link
Contributor

@EvanBacon EvanBacon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Update the changelog and merge 🙏

@danibonilha
Copy link
Contributor Author

Hey @brentvatne could you re-review it please?

@brentvatne brentvatne merged commit a4304b8 into expo:master Nov 14, 2019
@brentvatne
Copy link
Member

thanks @danibonilha!

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

Successfully merging this pull request may close these issues.

3 participants