Skip to content

Fix ifdef for shadow inconsistency bug fix #1378

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
Closed
wants to merge 2 commits into from
Closed

Fix ifdef for shadow inconsistency bug fix #1378

wants to merge 2 commits into from

Conversation

lyzhan7
Copy link

@lyzhan7 lyzhan7 commented Aug 25, 2022 •
edited
Loading

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

Fix regression for ifdef for shadow inconsistency bug fix (checked in a598ee4)

We were seeing a red box in iOS (copied below), after this fix confirmed that this box is gone.
MicrosoftTeams-image (2)

Changelog

[macOS] [Fixed] - Mixed up if defs for shadow inconsistency bug

@lyzhan7 lyzhan7 requested a review from a team as a code owner August 25, 2022 18:14
@lyzhan7 lyzhan7 enabled auto-merge (squash) August 25, 2022 18:14
harrieshin
harrieshin reviewed Aug 25, 2022
View reviewed changes
React/Views/RCTViewManager.m
@@ -197,7 +197,7 @@ - (RCTShadowView *)shadowView
RCT_REMAP_VIEW_PROPERTY(opacity, alphaValue, CGFloat)
#endif // ]TODO(macOS GH#774)

#if TARGET_OS_OSX // TODO(macOS GH#774)
#if !TARGET_OS_OSX // TODO(macOS GH#774)
Copy link

Choose a reason for hiding this comment

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

how did this work before? and nothing failed?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I had the same question, I didn't test on IOS when I made this change but I feel like I should have seen errors when I tested on macOS

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a copy-paste error since you tested in FURN and not RN-Tester?

@harrieshin
Copy link

please add description which commit has cause the regression

@harrieshin
Copy link

please add vlaidation notes

This was referenced Aug 25, 2022
Closed
Closed
@lyzhan7 lyzhan7 disabled auto-merge August 25, 2022 19:03
@lyzhan7
Copy link
Author

lyzhan7 commented Aug 25, 2022

The Apple PR pipeline is failing with this error:

"RNTesterLoadAllPages
AnExApp, RedBox errors: Error setting property 'shadowRadius' of RCTText with tag #13: Exception thrown while executing UI block: -[RCTTextView setShadowRadius:]: unrecognized selector sent to instance 0x617000940a80 (NSInternalInconsistencyException)".

This seems like an actual issue with the original PR, I think the mistake with ifdefs hid the issue and somehow the original PR passed these tests. This will need to be looked at more, so going to close this PR for now and revert the PR that introduced this until we find a fix.

@lyzhan7 lyzhan7 closed this Aug 25, 2022
@lyzhan7 lyzhan7 deleted the lyzhan-fix-ifdef branch March 16, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Saadnajmi Saadnajmi Saadnajmi left review comments

@harrieshin harrieshin harrieshin approved these changes

@chiuam chiuam chiuam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lyzhan7 @harrieshin @Saadnajmi @chiuam