Skip to content

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

Closed
Simek wants to merge 4 commits into
react:masterfrom
Simek:darkmode-in-newappscreen-and-template
Closed

Add Dark Mode support to the App template and NewAppScreen components#28711
Simek wants to merge 4 commits into
react:masterfrom
Simek:darkmode-in-newappscreen-and-template

Conversation

@Simek

@SimekSimek commented Apr 21, 2020

Copy link
Copy Markdown
Collaborator

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-botreact-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Apr 21, 2020
@facebook-github-botfacebook-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

analysis-bot commented Apr 21, 2020

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universal809,5680

Base commit: 2ad827b

@analysis-bot

analysis-bot commented Apr 21, 2020

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a6,761,242187
androidhermesarmeabi-v7a6,408,250193
androidhermesx867,143,453205
androidhermesx86_647,034,176204
androidjscarm64-v8a8,929,72537
androidjscarmeabi-v7a8,569,12958
androidjscx868,755,03931
androidjscx86_649,331,41250

Base commit: 2ad827b

@jacobp100

Copy link
Copy Markdown
Contributor

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

@Simek

Simek commented Apr 22, 2020

Copy link
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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

Simek commented Apr 22, 2020

Copy link
Copy Markdown
CollaboratorAuthor

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

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@react-native-bot

Copy link
Copy Markdown
Collaborator

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

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

@react-native-botreact-native-bot added the Merged This PR has been merged. label Apr 24, 2020
@Simek

Copy link
Copy Markdown
CollaboratorAuthor

@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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Type: EnhancementA new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@Simek@analysis-bot@jacobp100@shergin@react-native-bot@facebook-github-bot