Skip to content

Conversation

@AndriyGo
Copy link
Contributor

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@AndriyGo
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

LGTM, but had some doc comment changes (though it looks like these were existing docs bugs)

- (void)cancelAuthorization;

/** @fn showSignInAlertWithEmail:provider:handler:
@brief Displays an alert to conform with user whether she wants to proceed with the provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Displays an alert asking the user to confirm whether or not they want to proceed with the selected provider.


/** @fn incrementActivity
@brief Increment the current activity count. If there's positive number of activities, display
and animate the activity indicator with a short period of delay.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: with a short delay instead of with a short period of delay.

- (void)decrementActivity;

/** @fn addActivityIndicator:
@brief Creates and add activity indicator to the center of the specified view.
Copy link
Contributor

Choose a reason for hiding this comment

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

Creates and adds an activity indicator


/** @fn addActivityIndicator:
@brief Creates and add activity indicator to the center of the specified view.
@param view The View where indicator is shown.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: View should be lowercase

@AndriyGo
Copy link
Contributor Author

AndriyGo commented May 1, 2018

@morganchen12 👍

Do you have any idea when this can be added to the latest release?

@morganchen12
Copy link
Contributor

I'll merge it after the comment changes are made, and it'll be out in the the next release (after Firebase's next major version).

@AndriyGo
Copy link
Contributor Author

AndriyGo commented May 3, 2018

@morganchen12 sorry this is my first public PR – do I need to make the comment changes which you specified? Or do I need to approve them somehow?

@morganchen12
Copy link
Contributor

If you make the changes locally and push to the same branch, the PR will update automatically.

@morganchen12 morganchen12 merged commit 4229048 into firebase:master May 3, 2018
@morganchen12
Copy link
Contributor

Looks good, thanks! Expect this to be released in the next two weeks or so.

@AndriyGo
Copy link
Contributor Author

AndriyGo commented May 4, 2018

@morganchen12 thank you!

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.

3 participants