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

[iOS] Exclude Android HorizontalScrollContentView cxx component code #48138

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[iOS] Exclude Android HorizontalScrollContentView cxx component code #48138

wants to merge 2 commits into from
+3 −4

Conversation

zhongwuzw
Copy link
Contributor

@zhongwuzw zhongwuzw commented Dec 6, 2024 •
edited
Loading

Summary:

Exclude Android HorizontalScrollContentView cxx component code, iOS don't need it. cc @NickGerleman can you help to review? :)

Changelog:

[IOS] [FIXED] - Exclude Android HorizontalScrollContentView cxx component code

Test Plan:

iOS don't include any Android HorizontalScrollContentView code.

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 6, 2024
NickGerleman
NickGerleman reviewed Dec 10, 2024
View reviewed changes
packages/react-native/ReactCommon/React-FabricComponents.podspec Outdated
Comment on lines 140 to 141
sss.exclude_files = "react/renderer/components/scrollview/tests",
"react/renderer/components/scrollview/platform/android"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way we can invert this to be allowlist for iOS and base instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any way we can invert this to be allowlist for iOS and base instead?

Hi , can you explain more? should we share the Android horizontal scroll content view for iOS?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, instead of excluding tests, and android platform, we only include shared (not in nested in scrollview folder), and iOS specific files (those in platform/ios).

Some React Native forks end up adding additional platforms, so the pattern of excluding specific ones can work in RN repo, but cause issues for those forks (which then end up including code for multiple platforms).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickGerleman Got it :), I updates the podspec to only include shared files for iOS, because now we don't have iOS specific files. Please review it again.

@facebook-github-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NickGerleman NickGerleman NickGerleman 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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhongwuzw @facebook-github-bot @NickGerleman