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

Fix callbacks not being fired on macOS ActionSheetIOS #1830

Merged
merged 1 commit into from
May 25, 2023

Conversation

Saadnajmi
Copy link
Collaborator

@Saadnajmi Saadnajmi commented May 25, 2023

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

Resolves #1680
On macOS, we implement ActionSheetIOS to be an NSMenu (not confusing at all). During the 0.71 merge, RCTActionSheetManager.mm was refactored a bit, and the macOS implementation wasn't properly updated. Thus, callbacks wouldn't get fired when you clicked on menu items.

While the fix is simple (initialize _callbacks on init), I decided to go ahead and refactor the diffs so that the iOS and macOS blocks are much closer to each other and share more code. I also added a few comments explaining some macOS differences we needed. This should help ensure that future merges are less likely to break this module.

Things to note:

  • This PR also fixes the ability to disable menu items, which previously didn't work. Yay code sharing :D.
  • NSMenu doesn't have a "cancel" menu item like iOS Action Sheet buttons do, you just click away to close the menu. So, we just don't add a menu item for any button marked as "cancel"
  • NSMenuItems don't have a "destructive" style like iOS Action Sheet buttons do. So we don't do any special styling for them.
  • I didn't refactor any of the Share dialog bit because there isn't a bug there, and I decided to leave as is.

Changeling

[MACOS] [FIXED] - Fix callbacks not being fired on macOS ActionSheetIOS

Test Plan

See video. Before, clicking any menu item would result in a warning that no callback is registered.

Screen.Recording.2023-05-25.at.12.34.53.PM.mov

@Saadnajmi Saadnajmi requested a review from a team as a code owner May 25, 2023 19:43
@github-actions
Copy link

github-actions bot commented May 25, 2023

Fails
🚫

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • macOS
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 83c4eb6

@Saadnajmi Saadnajmi merged commit f80d1c0 into microsoft:main May 25, 2023
@Saadnajmi Saadnajmi deleted the menu branch May 25, 2023 21:11
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.

Refactor RCTActionSheetManager macOS implementation to not rely on removed _callbacks
2 participants