Skip to content

Merge AlertIOS with Alert #23318

Closed
wellmonge wants to merge 12 commits into
react:masterfrom
wellmonge:master
Closed

Merge AlertIOS with Alert #23318
wellmonge wants to merge 12 commits into
react:masterfrom
wellmonge:master

Conversation

@wellmonge

@wellmonge wellmonge commented Feb 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@analysis-bot analysis-bot left a comment

Copy link
Copy Markdown

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.

Comment thread Libraries/Alert/Alert.js Outdated
Comment thread Libraries/Alert/Alert.js Outdated
Comment thread Libraries/Alert/Alert.js Outdated
Comment thread Libraries/Alert/Alert.js Outdated
Comment thread Libraries/Alert/Alert.js Outdated
Comment thread Libraries/Alert/Alert.js Outdated
Comment thread Libraries/Alert/Alert.js Outdated
Comment thread Libraries/Alert/Alert.js Outdated
Comment thread Libraries/Alert/Alert.js Outdated
Comment thread Libraries/Alert/Alert.js Outdated
@pull-bot

pull-bot commented Feb 6, 2019

Copy link
Copy Markdown
Warnings
⚠️

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

Generated by 🚫 dangerJS

cpojer
cpojer reviewed Feb 6, 2019
View reviewed changes
Comment thread Libraries/react-native/react-native-implementation.js
cpojer
cpojer suggested changes Feb 6, 2019
View reviewed changes

@cpojer cpojer left a comment

Copy link
Copy Markdown
Contributor

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

wellmonge commented Feb 6, 2019

Copy link
Copy Markdown
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
Comment thread Libraries/Alert/Alert.js Outdated
@cpojer cpojer self-assigned this Feb 7, 2019
wellmonge
wellmonge commented Feb 9, 2019
View reviewed changes

@wellmonge wellmonge left a comment

Copy link
Copy Markdown
Contributor Author

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

@wellmonge wellmonge left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

wellmonge commented Feb 11, 2019

Copy link
Copy Markdown
Contributor Author

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

@guhungry

guhungry commented Feb 11, 2019

Copy link
Copy Markdown
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

guhungry commented Feb 11, 2019

Copy link
Copy Markdown
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

wellmonge commented Feb 11, 2019

Copy link
Copy Markdown
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

@cpojer cpojer left a comment

Copy link
Copy Markdown
Contributor

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

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

Copy link
Copy Markdown
Contributor

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

wellmonge commented Feb 11, 2019

Copy link
Copy Markdown
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

react-native-bot commented Feb 12, 2019

Copy link
Copy Markdown
Collaborator

@wellmonge merged commit e2bd7db into facebook:master.

@react react 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: react/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

4 more reviewers

@guhungry guhungry guhungry left review comments

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

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

@cpojer cpojer cpojer approved these changes

Reviewers whose approvals may not affect merge requirements

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