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

Conditionally fires onDismiss when receiving ContentDialogResult::None #8866

Merged
merged 2 commits into from
Oct 27, 2021

Conversation

rozele
Copy link
Collaborator

@rozele rozele commented Oct 20, 2021

When the ContentDialog returns with a result of None, and there is no close button / neutral button option presented to the user, this change fires an onDismiss instead of attempting to fire the buttonNeutral.onPress callback, which is very unlikely to be registered.

It is very unlikely that RNW apps have leaned on this, but if someone is depending on the implementation detail of the "neutral" callback being called without presenting the button (by leaving the text for the neutral button option null), then this is a minor breaking change.

Fixes #8865

Microsoft Reviewers: Open in CodeFlow

…None`

When the ContentDialog returns with a result of `None`, and there is no
close button / neutral button option presented to the user, this change
fires an `onDismiss` instead of attempting to fire the
`buttonNeutral.onPress` callback, which is very unlikely to be
registered.

It is very unlikely that RNW apps have leaned on this, but if someone is
depending on the implementation detail of the "neutral" callback being
called without presenting the button (by leaving the text for the
neutral button option null), then this is a minor breaking change.

Fixes microsoft#8865
@rozele rozele requested a review from a team as a code owner October 20, 2021 03:34
@asklar asklar added the Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release label Oct 20, 2021
@rozele
Copy link
Collaborator Author

rozele commented Oct 20, 2021

As I said, here is an example where this is technically a breaking change:

Alert.alert('Alert', 'Warning, an alert is being shown!', [
  {onPress: () => setMessage('Neutral Pressed!')},
  {text: 'No'},
  {text: 'Yes'},
])

See before that the "Neutral Pressed!" log is displayed:

playground.2021-10-19.23-51-37.mp4

After, the callback is no longer invoked.

playground.2021-10-19.23-49-50.mp4

However, we now have onDismiss functionality, so the following code produces a "Dismissed!" log:

Alert.alert('Alert', 'Warning, an alert is being shown!', [
    {text: 'No'},
    {text: 'Yes'},
  ], {
    onDismiss: () => setMessage('Dismissed!'),
  })
playground.2021-10-19.23-48-28.mp4

@NickGerleman NickGerleman merged commit cc7095c into microsoft:main Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Alert should fire onDismiss when no Close option is available
3 participants