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

Dark mode support for RCTPerfMonitor #28130

Closed
wants to merge 8 commits into from
Closed

Dark mode support for RCTPerfMonitor #28130

wants to merge 8 commits into from

Conversation

gorhom
Copy link
Contributor

@gorhom gorhom commented Feb 19, 2020

Summary

Hi There 👋,

While I'm developing on iOS with dark appearance enabled, i notice that Pref Monitor view doesn't support dark mode yet, so here is the PR to fix it :)

Changelog

[iOS] [Fixed] - Fix Pref Monitor in dark appearance

Test Plan

Run any React Native app on iOS > Turn on dark appearance

Before

Before

After

After

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 19, 2020
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Feb 19, 2020
@analysis-bot
Copy link

RNTester (Android/hermes/arm64-v8a): 3289088 bytes
RNTester (Android/hermes/armeabi-v7a): 3129344 bytes
RNTester (Android/hermes/x86): 3461120 bytes
RNTester (Android/hermes/x86_64): 3424256 bytes
RNTester (Android/jsc/arm64-v8a): 4446208 bytes
RNTester (Android/jsc/armeabi-v7a): 4268032 bytes
RNTester (Android/jsc/x86): 4358144 bytes
RNTester (Android/jsc/x86_64): 4644864 bytes

@analysis-bot
Copy link

RNTester.app (iOS): 10657792 bytes

Copy link
Contributor

@hramos hramos left a comment

Choose a reason for hiding this comment

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

This might work well in Dark Mode, but I suspect it breaks Light Mode. Can you use semantic colors instead? Here's a quick guide I've referred to in the past: https://nshipster.com/dark-mode/

@gorhom
Copy link
Contributor Author

gorhom commented Feb 19, 2020

hi @hramos , thanks for taking a time to review this.

by reverting my commits and add systemBackgroundColor to the root container

if (@available(iOS 13.0, *)) {
  _container.backgroundColor = UIColor.systemBackgroundColor;
}

i got this :
Dark

is this okay ?

@gorhom
Copy link
Contributor Author

gorhom commented Feb 20, 2020

hi @hramos , i have reverted my previous commits and only set the root container of RCTPerfMonitor background to UIColor.systemBackgroundColor, and this is how it looks in

Light
light
Dark
dark

@gorhom gorhom requested a review from hramos February 20, 2020 18:43
@analysis-bot
Copy link

RNTester (Android/hermes/arm64-v8a): 3289088 bytes
RNTester (Android/hermes/armeabi-v7a): 3129344 bytes
RNTester (Android/hermes/x86): 3461120 bytes
RNTester (Android/hermes/x86_64): 3424256 bytes
RNTester (Android/jsc/arm64-v8a): 4446208 bytes
RNTester (Android/jsc/armeabi-v7a): 4268032 bytes
RNTester (Android/jsc/x86): 4358144 bytes
RNTester (Android/jsc/x86_64): 4644864 bytes

@analysis-bot
Copy link

RNTester.app (iOS): 10657792 bytes

@PeteTheHeat PeteTheHeat changed the title Pref Monitor in dark appearance on iOS Dark mode support for RCTPerfMonitor Feb 24, 2020
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@PeteTheHeat
Copy link
Contributor

This looks good! Thanks for the PR.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@PeteTheHeat
Copy link
Contributor

Although this probably breaks anything pre iOS13. When I land it, I'll add another background color for that.

@gorhom
Copy link
Contributor Author

gorhom commented Feb 25, 2020

@PeteTheHeat thanks for taking a time to review this :)

about worrying about breaking pre iOS 13, i think it should be fine since wrapped the background colour with available(iOS 13.0, *) annotation if-condition. Also pre iOS 13 has only light mode.

@PeteTheHeat
Copy link
Contributor

But that line adding white background color was important. After this diff, on iOS 12, the background color will be transparent. I'm thinking:

if (@available(iOS 13.0, *)) { _container.backgroundColor = UIColor.systemBackgroundColor; } else { _container.backgroundColor = UIColor.whiteColor; }

@gorhom
Copy link
Contributor Author

gorhom commented Feb 25, 2020

@PeteTheHeat , ohh yeah sure i will push it right now

@analysis-bot
Copy link

RNTester (Android/hermes/arm64-v8a): 3289088 bytes
RNTester (Android/hermes/armeabi-v7a): 3129344 bytes
RNTester (Android/hermes/x86): 3461120 bytes
RNTester (Android/hermes/x86_64): 3424256 bytes
RNTester (Android/jsc/arm64-v8a): 4446208 bytes
RNTester (Android/jsc/armeabi-v7a): 4268032 bytes
RNTester (Android/jsc/x86): 4358144 bytes
RNTester (Android/jsc/x86_64): 4644864 bytes

@analysis-bot
Copy link

RNTester.app (iOS): 10657792 bytes

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gorhom in 576ddfb.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 28, 2020
osdnk pushed a commit to osdnk/react-native that referenced this pull request Mar 9, 2020
Summary:
Hi There 👋,

While I'm developing on iOS with dark appearance enabled, i notice that `Pref Monitor` view doesn't support dark mode yet, so here is the PR to fix it :)

## Changelog

[iOS] [Fixed] - Fix Pref Monitor in dark appearance
Pull Request resolved: facebook#28130

Test Plan:
Run any React Native app on iOS > Turn on dark appearance
### Before
![Before](https://user-images.githubusercontent.com/4061838/74872974-8b75b600-535e-11ea-8550-763a598547ec.png)
### After
![After](https://user-images.githubusercontent.com/4061838/74872978-8ca6e300-535e-11ea-9862-4d3014e849f7.png)

Reviewed By: RSNara

Differential Revision: D20080019

Pulled By: PeteTheHeat

fbshipit-source-id: 9365daa3f7193a11760bc1372b8de2c3896def5c
alloy pushed a commit to alloy/react-native that referenced this pull request Apr 3, 2020
Summary:
Hi There 👋,

While I'm developing on iOS with dark appearance enabled, i notice that `Pref Monitor` view doesn't support dark mode yet, so here is the PR to fix it :)

## Changelog

[iOS] [Fixed] - Fix Pref Monitor in dark appearance
Pull Request resolved: facebook#28130

Test Plan:
Run any React Native app on iOS > Turn on dark appearance
### Before
![Before](https://user-images.githubusercontent.com/4061838/74872974-8b75b600-535e-11ea-8550-763a598547ec.png)
### After
![After](https://user-images.githubusercontent.com/4061838/74872978-8ca6e300-535e-11ea-9862-4d3014e849f7.png)

Reviewed By: RSNara

Differential Revision: D20080019

Pulled By: PeteTheHeat

fbshipit-source-id: 9365daa3f7193a11760bc1372b8de2c3896def5c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 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. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants