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

Replace global.alert use to fix eslint warnings #22184

Closed
wants to merge 1 commit into from
Closed

Replace global.alert use to fix eslint warnings #22184

wants to merge 1 commit into from

Conversation

vcalvello
Copy link
Contributor

@vcalvello vcalvello commented Nov 6, 2018 •
edited
Loading

Replaces alert with Alert.alert to fix eslint warnings.

Test Plan:

yarn lint

Check no-alert lint warning displayed.

Changelog:

[INTERNAL] [ENHANCEMENT] - Replace alert with Alert.alert to fix eslint warning.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. ✅pr adds tests labels Nov 6, 2018
@pull-bot
Copy link

pull-bot commented Nov 6, 2018

Warnings
⚠️

📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

elicwhite
elicwhite reviewed Nov 6, 2018
View reviewed changes
RNTester/js/AccessibilityIOSExample.js Outdated

class AccessibilityIOSExample extends React.Component<{}> {
render() {
return (
<View>
<View
onAccessibilityTap={() => alert('onAccessibilityTap success')}
onAccessibilityTap={() => Alert.alert('onAccessibilityTap success')}
Copy link
Member

Choose a reason for hiding this comment

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

This is almost the same. global's alert sets the title of the alert to "Alert". https://github.com/facebook/react-native/blob/master/Libraries/Core/setUpAlert.js#L20

@RSNara
Copy link
Contributor

RSNara commented Nov 8, 2018

What's the rationale behind removing reliance on global.alert? It seems like a fairly innocent/standard function that provides the convenience of calling Alert.alert in a fairly standardized manner. Considering how all browsers implement this function, I'm not really sure why we should diverge and try to get rid of it. Is that what the intent of this PR is?

@elicwhite
Copy link
Member

I think the intent is to fix the eslint warning which is pretty standard to disallow alert and console calls. I think the question is do we disable the linter for this file, or throughout all of RNTester, or use the Alert module which doesn’t trigger the lint. shrug

@vcalvello
Copy link
Contributor Author

The idea is to fix the eslint warning. I'm not pretty sure how, though. I usually use alert and console calls for debugging purposes and I prefer not to disable linting rules, that's why I chose using Alert.alert. Anyway we can disable linting in that line with /*eslint-disable-line no-alert*/. Definitely, I wouldn't disable the linter for the whole RNTester project. What do you prefer?

@vcalvello vcalvello changed the title Replace alert with Alert API Replace global.alert use to fix eslint warnings Nov 9, 2018
RSNara
RSNara approved these changes Nov 16, 2018
View reviewed changes
Copy link
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

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

I think this is reasonable. 🤔

facebook-github-bot
facebook-github-bot reviewed Nov 16, 2018
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.

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

@react-native-bot
Copy link
Collaborator

@vcalvello merged commit 55994f5 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Nov 20, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Nov 20, 2018
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Apr 14, 2019
@vcalvello @facebook-github-bot
Replace global.alert use to fix eslint warnings (#22184)
404cdf2 
Summary:
Replaces `alert` with `Alert.alert` to fix eslint warnings.
Pull Request resolved: facebook/react-native#22184

Reviewed By: TheSavior

Differential Revision: D13105636

Pulled By: RSNara

fbshipit-source-id: 82a9e55fd002051e3cf8238e29d37b2b33f66f0e
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
@vcalvello @facebook-github-bot
Replace global.alert use to fix eslint warnings (facebook#22184)
fad5357 
Summary:
Replaces `alert` with `Alert.alert` to fix eslint warnings.
Pull Request resolved: facebook#22184

Reviewed By: TheSavior

Differential Revision: D13105636

Pulled By: RSNara

fbshipit-source-id: 82a9e55fd002051e3cf8238e29d37b2b33f66f0e
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

@elicwhite elicwhite elicwhite left review comments

@RSNara RSNara RSNara approved these changes

Assignees
No one assigned
Labels
API: Alert 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vcalvello @pull-bot @RSNara @elicwhite @react-native-bot @facebook-github-bot @hramos