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

Add Dark Mode support to the App template and NewAppScreen components #28711

Closed

Conversation

Simek
Copy link
Collaborator

@Simek Simek commented Apr 21, 2020

Summary

This PR adds support for the dark mode and dynamic theme changing to the default App template and to the related NewAppScreen components. Using useColorScheme hook forced me to refactor a bit main App.js file, but I think those changes are step in the right direction according to way in which React Native is used in larger apps, so new Section component has been extracted to reduce code redundancy/repetition inside App.

Additional color darker has been added to the Colors statics from NewAppScreen because dark was too bright for the Dark Mode backgrounds.

Also main StoryBoard on iOS has been updated to use theme based colors instead of static or hardcoded ones. There was also an unused, empty Label which I have removed.

I'm not so much experienced with Android. If someone could also update Android splash screen (if Android requires such change) it will be nice. I want to look at this later using simulator.

I have updated the Android splash screen and tested this change on the Android emulator.

If you have any comment or corrections feel free to post them out, I would like to put more work into this PR if it's needed. Dark Mode this days is a part of near every OS, so it could be considered as a standard feature. I hope those changes helps people which struggle with the basic theming implementation (+ there is now an example of hook and children prop usage in the template).

Changelog

[Internal] [Added] - Add dark mode support to the default app template

Test Plan

I have tested the App from the template on the iOS device and in Android emulator with RN 0.63.0-rc.

Screen recording on iOS (demonstarates both modes, both splash screens and transition):
ezgif-6-e24ee8e839c9

Screenshot of iOS app in Dark Mode:
IMG_6542

Screenshot of iOS app splash screen in Dark Mode:
IMG_6544

Screenshot of Android app in the emulator:
Screenshot_1587566100

@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Apr 21, 2020
@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 Apr 21, 2020
@analysis-bot
Copy link

analysis-bot commented Apr 21, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal 809,568 0

Base commit: 2ad827b

@analysis-bot
Copy link

analysis-bot commented Apr 21, 2020

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 6,761,242 187
android hermes armeabi-v7a 6,408,250 193
android hermes x86 7,143,453 205
android hermes x86_64 7,034,176 204
android jsc arm64-v8a 8,929,725 37
android jsc armeabi-v7a 8,569,129 58
android jsc x86 8,755,039 31
android jsc x86_64 9,331,412 50

Base commit: 2ad827b

@jacobp100
Copy link
Contributor

You might want to hold off doing this until PlatformColor has landed - then you can support dark mode inside StylesSheet.create

@Simek
Copy link
Collaborator Author

Simek commented Apr 22, 2020

Thank you for your comment. I have mentioned using Platform Color API in my initial comment in the 0.63 release discussion thread.

But after thinking and researching a while it looks like, at least for now, there is no possibility to remove Colors statics from the NewAppScreen due to compatibility with out-of-tree platforms. I'm also not sure if using Platform Color simplify or make more complex the code, because you need to define color for at least 2 platforms + default and for custom colors or StatusBar the hook will be still required. It's also brand new API so I think it's wise to wait with this improvement a bit.

Of course if such changes will be request I'm happy to implement them.

@Simek
Copy link
Collaborator Author

Simek commented Apr 22, 2020

BTW. I have noticed that after updating Android AppTheme even JS bundle loading view has dark background, while on iOS Dark Mode do not affect the background color and in both modes it's white.

@jacobp100
Copy link
Contributor

In AppDelegate.m, you could change the root background colour code to something like

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

@Simek
Copy link
Collaborator Author

Simek commented Apr 22, 2020

@jacobp100 Awesome tip, I forget about updating RootView. This change fixes the issue described in the comment above, thank you!

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.

@shergin is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@shergin
Copy link
Contributor

shergin commented Apr 24, 2020

Looks so pretty! I can't resist!
Thank you!

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @Simek in 2b56011.

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 Apr 24, 2020
@Simek
Copy link
Collaborator Author

Simek commented Apr 28, 2020

@shergin One more thing that came to my mind:

  • Can I move Hermes badge to the NewAppScreen components or it is important to expose global.HermesInternal for the template users?

facebook-github-bot pushed a commit that referenced this pull request Jul 14, 2020
Summary:
Refs #28711

This PR moves Hermes badge component from the template to the `NewAppScreen` library. The main motivation behind this change was to simplify a bit template code.

I assumed that it is not important to expose `global.HermesInternal` to the template users:
* If this assumption is true, I think that there are no other reason to leave this component inside `App` in comparison to other `NewAppScreen` components,
* If this assumption is false, I can adjust this PR and move `HermesInternal` check from the badge component to the `App`.

I was trying to avoid calling `useColorScheme` when Hermes is disabled, but placing hook inside the conditional branch causes ESLint warning (react-hooks/rules-of-hooks).

This PR includes also small style tweaks for the badge - since there are no background padding can be omitted and spacing can be added adjusted tweaking `top` and `left` properties and `fontSize` has been adjusted just for the readability.

In the last commit, I have gone a bit further and moved `HermesBadge` to the `Header` component and I have also changed slightly the `Header` title (React -> React Native) and it's styling.
> I'm not sure if after this change `HermesBadge` export in `NewAppScreen` components list is still required, but maybe this badge will be useful for someone. If it's a mistake I can update the PR and remove this export.

## Changelog

[Internal][Changed] move Hermes badge from the template to the NewAppScreen library

Pull Request resolved: #28783

Test Plan:
Template app do not redbox on Android emulator with and without Hermes enabled.

## Preview
Android with Hermes enabled and adjusted header:

![Screenshot_1588164908](https://user-images.githubusercontent.com/719641/80599357-16dc8900-8a2b-11ea-8b3e-9a2cb26d3470.png)

iOS with adjusted header:

![IMG_6551](https://user-images.githubusercontent.com/719641/80599445-3bd0fc00-8a2b-11ea-8215-318625ddad13.PNG)

Reviewed By: GijsWeterings

Differential Revision: D22493822

Pulled By: cpojer

fbshipit-source-id: 3440e10f2d59f268ca8851a6e002f0ff23fa839c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants