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

Fix Catalyst @availablity checks in RCTPullToRefreshViewComponentView.mm #35673

Closed
wants to merge 4 commits into from
Closed

Fix Catalyst @availablity checks in RCTPullToRefreshViewComponentView.mm #35673

wants to merge 4 commits into from

Conversation

Saadnajmi
Copy link
Contributor

@Saadnajmi Saadnajmi commented Dec 19, 2022 •
edited
Loading

Summary

Judging from the original commit, the availability check is probably for Catalyst, not macOS. That matches the apple documentation as well (though we need 13.1, not 13).

To avoid needing to introduce a diff in React Native macOS (where we actually compile for macOS and not Mac Catalyst) let's fix the availability check to be more accurate. Changing macOS to MacCatalyst should be supported as per https://docs.swift.org/swift-book/ReferenceManual/Attributes.html#ID348

Changelog

[IOS] [FIXED] - Fixed Mac Catalyst availability checks

Test Plan

Code compiles

@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. p: Microsoft Partner: Microsoft Partner labels Dec 19, 2022
@analysis-bot
Copy link

analysis-bot commented Dec 19, 2022 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,470,557 +3,835
android hermes armeabi-v7a 7,789,710 +3,737
android hermes x86 8,945,115 +4,452
android hermes x86_64 8,802,835 +4,211
android jsc arm64-v8a 9,656,818 +4,009
android jsc armeabi-v7a 8,389,929 +3,921
android jsc x86 9,720,822 +4,626
android jsc x86_64 10,198,308 +4,386

Base commit: 9f78517
Branch: main

@analysis-bot
Copy link

analysis-bot commented Dec 19, 2022 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 9f78517
Branch: main

@Saadnajmi Saadnajmi changed the title Fix iOS @availablity checks Fix iOS @availablity checks in RCTPullToRefreshViewComponentView.mm Dec 19, 2022
@pull-bot
Copy link

PR build artifact for aee7e0a is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@Saadnajmi Saadnajmi mentioned this pull request Dec 19, 2022
Merged
4 tasks
@Saadnajmi
Copy link
Contributor Author

Update: Judging from the original commit, the availability check is probably for Catalyst, not macOS. I'll update this PR to reflect that. Thanks @anandrajeswaran for the catch!

@Saadnajmi Saadnajmi changed the title Fix iOS @availablity checks in RCTPullToRefreshViewComponentView.mm Fix Catalyst @availablity checks in RCTPullToRefreshViewComponentView.mm Dec 19, 2022
cipolleschi
cipolleschi approved these changes Dec 19, 2022
View reviewed changes
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

@Saadnajmi thank you for spotting this!

@facebook-github-bot
Copy link
Contributor

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

@pull-bot
Copy link

PR build artifact for 2700551 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 35978a5 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@Saadnajmi
Copy link
Contributor Author

@cancan101 anything else I need to do to get this merged?

@cancan101
Copy link
Contributor

@Saadnajmi why are you asking me ;-) ? Not sure I am the right person here to sign off.

@Saadnajmi
Copy link
Contributor Author

@Saadnajmi why are you asking me ;-) ? Not sure I am the right person here to sign off.

Tagged the wrong person, sorry! Meant to be @cipolleschi

@facebook-github-bot
Copy link
Contributor

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

@cipolleschi
Copy link
Contributor

Nope! I'm trying to land this right now. Sorry, but with the Holidays coming, it is a slower period.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Dec 30, 2022
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 70d9b56.

@Saadnajmi Saadnajmi deleted the patch-1 branch January 15, 2023 23:46
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
@Saadnajmi @OlimpiaZurek
Fix Catalyst @availablity checks in RCTPullToRefreshViewComponentView…
74a5117 
….mm (facebook#35673)

Summary:
Judging from the original [commit](facebook@4c4948b), the availability check is probably for Catalyst, not macOS.  That matches the [apple documentation](https://developer.apple.com/documentation/uikit/uiscrollview/2127691-refreshcontrol?language=objc) as well (though we need 13.1, not 13).

To avoid needing to introduce a diff in React Native macOS (where we actually compile for macOS and not Mac Catalyst) let's fix the availability check to be more accurate. Changing `macOS` to `MacCatalyst` should be supported as per https://docs.swift.org/swift-book/ReferenceManual/Attributes.html#ID348 (facebook@2c5a966)

## Changelog

[IOS] [FIXED] - Fixed Mac Catalyst availability checks

Pull Request resolved: facebook#35673

Test Plan: Code compiles

Reviewed By: christophpurrer

Differential Revision: D42149589

Pulled By: cipolleschi

fbshipit-source-id: fbbd31cba44f55215f2c4c0f37aad410d5bcd627
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cipolleschi cipolleschi cipolleschi approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@Saadnajmi @analysis-bot @pull-bot @facebook-github-bot @cancan101 @cipolleschi