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.

Sign up for GitHub

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

Jump to bottom

Merge AlertIOS with Alert #23318

Closed
wants to merge 12 commits into from
Closed

Merge AlertIOS with Alert #23318

wants to merge 12 commits into from

Conversation

wellmonge
Copy link
Contributor

@wellmonge wellmonge commented Feb 6, 2019 •
edited
Loading

Summary

Itwas merged AlertIOS into Alert and removed type parameter from Alert.alert line 60 at Alert.js

Changelog

[AlertIOS] [Change and Replace] - Merge AlertIOS into Alert.

Test Plan

CI is green, and everything should work as normal.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 6, 2019
analysis-bot
analysis-bot suggested changes Feb 6, 2019
View reviewed changes
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.
  • flow found some issues.

Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
@pull-bot
Copy link

pull-bot commented Feb 6, 2019

Warnings
⚠️

📋 Changelog Format - Did you include a Changelog? A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

Generated by 🚫 dangerJS

cpojer
cpojer suggested changes Feb 6, 2019
View reviewed changes
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Seems like there is gonna be a bit more work necessary to pull this off :)

@wellmonge
Copy link
Contributor Author

Seems like there is gonna be a bit more work necessary to pull this

@wellmonge wellmonge closed this Feb 6, 2019
@wellmonge wellmonge reopened this Feb 6, 2019
guhungry
guhungry reviewed Feb 7, 2019
View reviewed changes
Libraries/Alert/Alert.js Outdated Show resolved Hide resolved
@cpojer cpojer self-assigned this Feb 7, 2019
wellmonge
wellmonge commented Feb 9, 2019
View reviewed changes
Copy link
Contributor Author

@wellmonge wellmonge left a comment

Choose a reason for hiding this comment

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

Now I think everithing it's in the right place.

wellmonge
wellmonge commented Feb 9, 2019
View reviewed changes
Copy link
Contributor Author

@wellmonge wellmonge left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm still getting error

@wellmonge
Copy link
Contributor Author

@guhungry, @cpojer could anyone help with those failure at CI js_coverage?

@guhungry
Copy link
Contributor

@guhungry, @cpojer could anyone help with those failure at CI js_coverage?

Why it was run on different circle account
https://circleci.com/gh/wellmonge/react-native/118?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
vs where it should be
https://circleci.com/gh/facebook/react-native/tree/pull%2F23318

@guhungry
Copy link
Contributor

@wellmonge If you can trigger build for 4b53dd9 at https://circleci.com/gh/facebook/react-native/tree/pull%2F23318. I think all ci will be pass.

@wellmonge
Copy link
Contributor Author

@wellmonge If you can trigger build for 4b53dd9 at https://circleci.com/gh/facebook/react-native/tree/pull%2F23318. I think all ci will be pass.

Okay, later I'll do that

cpojer
cpojer approved these changes Feb 11, 2019
View reviewed changes
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

The code looks good! I reverted a few of the unintentional changes but we should be able to land this at FB soon.

facebook-github-bot
facebook-github-bot reviewed Feb 11, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@wellmonge
Copy link
Contributor Author

The code looks good! I reverted a few of the unintentional changes but we should be able to land this at FB soon.

Okay, let me know if had to do some other changes.

@react-native-bot
Copy link
Collaborator

@wellmonge merged commit e2bd7db into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 12, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 12, 2019
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Jul 18, 2019
@wellmonge @facebook-github-bot
Merge AlertIOS with Alert (#23318)
4377e4a 
Summary:
Itwas merged AlertIOS into Alert and removed type parameter from Alert.alert line 60 at Alert.js

[AlertIOS] [Change and Replace] - Merge AlertIOS into Alert.
Pull Request resolved: facebook/react-native#23318

Reviewed By: mjesun

Differential Revision: D14031421

Pulled By: cpojer

fbshipit-source-id: 98db173adeb65aa90d309f8a583993bc0cddb6e1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@guhungry guhungry guhungry left review comments

@analysis-bot analysis-bot analysis-bot requested changes

@cpojer cpojer cpojer approved these changes

Assignees

@cpojer cpojer

Labels
API: Alert API: AlertIOS CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wellmonge @pull-bot @guhungry @react-native-bot @cpojer @facebook-github-bot @analysis-bot