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

Issue #252 - BraintreeFragment with child FragmentManager. #253

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

zsoltvilagos
Copy link
Contributor

Solution for issue #252 , attaching BraintreeFragment to a child FragmentManager so that the instance can stay in the scope of a Fragment hosted by an Activity.

Copy link
Contributor

@quinnjn quinnjn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I had one comment about an API approvement for Fragment users. Otherwise this looks ready to roll!

return newInstance(activity, activity.getSupportFragmentManager(), authorization);
}

public static BraintreeFragment newInstance(Context context, FragmentManager fragmentManager, String authorization)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about a public method for passing a Fragment?

This function would be a private method that both our public newInstance would delegate to.

Choose a reason for hiding this comment

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

Haha. Actually, that was my initial idea. I thought you'd like this one as the public API. Making the change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@zsoltvilagos zsoltvilagos force-pushed the master branch 2 times, most recently from e68c925 to 2d214f9 Compare May 28, 2019 14:40
Copy link
Contributor

@quinnjn quinnjn left a comment

Choose a reason for hiding this comment

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

It looks like the latest addition broke a test where we pass null as the first argument.

You should be able to get around this by duplicating the test and pass a casted null for those tests.

Once that is fixed, we'll merge this in!

@zsoltvilagos
Copy link
Contributor Author

Fixed. Green again.

@zsoltvilagos
Copy link
Contributor Author

@quinnjn Any news? If I might ask.

@scannillo
Copy link
Contributor

Hi @zsoltvilagos. Looks like those additions to the BraintreeFragmentUnitTest.java file do fix the test issue we saw.

Our main Android dev @quinnjn has been busy helping with some other teams, so I don't want to merge without his final stamp of approval!

@zsoltvilagos
Copy link
Contributor Author

@scannillo Thx for the update.

@quinnjn
Copy link
Contributor

quinnjn commented Jun 5, 2019

Sorry for the delay. Thanks for working on this!

@quinnjn quinnjn merged commit 3a30946 into braintree:master Jun 5, 2019
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.

4 participants