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 nested Text components not inheriting foreground color from parent #1283

Merged
merged 1 commit into from
Jul 29, 2022

Conversation

christophpurrer
Copy link

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

This seems to due RN Mac commits that fix text colors for dark mode:

It works on iOS, web .... so it should behave the same on macOS as well

Changelog

[macOS] [Fixed] - Fix nested Text components not inheriting foreground color from parent

Test Plan

Before
before

After
after

iOS - as reference
Screen Shot 2022-07-25 at 9 21 58 AM

@christophpurrer christophpurrer requested a review from a team as a code owner July 25, 2022 16:22
@Saadnajmi Saadnajmi self-assigned this Jul 26, 2022
@christophpurrer christophpurrer force-pushed the fixNestedTextComponent branch from 187628c to b126bc5 Compare July 26, 2022 19:03
This seems to due RN Mac commits that fix text colors for dark mode:
- microsoft@8ed55a8
- microsoft@731a535

It works on iOS, web .... so it should behave the same on macOS as well
@christophpurrer christophpurrer force-pushed the fixNestedTextComponent branch from b126bc5 to e493785 Compare July 26, 2022 19:13
@Saadnajmi
Copy link
Collaborator

OK, so I looked more into this. I was wondering why RN macOS diverged from RN Core on foreground color for Text. Upon testing, I realize that RN Core has a bug that we fixed in RN macOS.

The iPhone simulator on the left and macOS app in the middle are running with React Native macOS. They properly handle Text in Dark mode. The iPhone simulator on the right is RN Core on main, and has the incorrect color (black text on dark grey background).

Screen Shot 2022-07-27 at 4 43 08 PM

Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

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

Between this PR and the 2 commits mentioned, there's a candidate for an RN Core upstream fix. I guess we can further discuss how we should track those / the strategy for how we push those / who will make the fix. I believe you mentioned there are more commits y'all have that could also be RN Core upstream fixes, so I feel it's worth chatting about :)

@christophpurrer
Copy link
Author

@Saadnajmi
So it seems we have 2 issues here:
1 rn-macOS doesn't correctly support inner text field colors
2 rn-core/iOS doesn't correctly support background colors

1 is fixed here. It 2 is still a problem I am gonna fix it as well ;-)

@Saadnajmi Saadnajmi merged commit bea60b7 into microsoft:main Jul 29, 2022
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
microsoft#1283)

This seems to due RN Mac commits that fix text colors for dark mode:
- microsoft@8ed55a8
- microsoft@731a535

It works on iOS, web .... so it should behave the same on macOS as well

Co-authored-by: Liron Yahdav <[email protected]>
# Conflicts:
#	Libraries/Text/RCTTextAttributes.m
#	packages/rn-tester/js/examples/Text/TextExample.ios.js

# Conflicts:
#	packages/rn-tester/js/examples/Text/TextExample.ios.js
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Mar 10, 2023
microsoft#1283)

This seems to due RN Mac commits that fix text colors for dark mode:
- microsoft@8ed55a8
- microsoft@731a535

It works on iOS, web .... so it should behave the same on macOS as well

Co-authored-by: Liron Yahdav <[email protected]>
# Conflicts:
#	Libraries/Text/RCTTextAttributes.m
#	packages/rn-tester/js/examples/Text/TextExample.ios.js

# Conflicts:
#	packages/rn-tester/js/examples/Text/TextExample.ios.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants