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: Allow to use PlatformColor in the theme #11570

Merged
merged 2 commits into from
Sep 2, 2023

Conversation

retyui
Copy link
Contributor

@retyui retyui commented Aug 31, 2023

Fix: #11481

@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (9ee55bc) 75.60% compared to head (c437e95) 75.60%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11570   +/-   ##
=======================================
  Coverage   75.60%   75.60%           
=======================================
  Files         194      194           
  Lines        5783     5783           
  Branches     2275     2275           
=======================================
  Hits         4372     4372           
  Misses       1364     1364           
  Partials       47       47           
Files Changed Coverage Δ
packages/bottom-tabs/src/views/BottomTabItem.tsx 63.46% <ø> (ø)
packages/bottom-tabs/src/views/TabBarIcon.tsx 100.00% <ø> (ø)
packages/drawer/src/views/DrawerItem.tsx 62.85% <ø> (ø)
packages/elements/src/Header/HeaderTitle.tsx 100.00% <ø> (ø)
packages/elements/src/MissingIcon.tsx 100.00% <ø> (ø)
packages/react-native-tab-view/src/TabBar.tsx 58.54% <ø> (ø)
packages/react-native-tab-view/src/TabBarItem.tsx 81.33% <0.00%> (ø)
...ages/react-native-tab-view/src/TabBarItemLabel.tsx 28.57% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@netlify
Copy link

netlify bot commented Aug 31, 2023

Deploy Preview for react-navigation-example ready!

Name Link
🔨 Latest commit c437e95
🔍 Latest deploy log https://app.netlify.com/sites/react-navigation-example/deploys/64f39de4a477c50008b00223
😎 Deploy Preview https://deploy-preview-11570--react-navigation-example.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@retyui retyui force-pushed the retyui/fix-11481 branch 3 times, most recently from 5cbf026 to c033310 Compare August 31, 2023 13:24
Copy link
Member

@osdnk osdnk left a comment

Choose a reason for hiding this comment

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

Hi,
Thanks for your PR. Please, next time provide a deeper description of your changes. I didn't understand initially how that change basically "propagates" from rn-screens :)

@osdnk osdnk enabled auto-merge (squash) September 2, 2023 20:41
@osdnk osdnk disabled auto-merge September 2, 2023 20:48
@osdnk osdnk merged commit 64734e7 into react-navigation:main Sep 2, 2023
8 checks passed
@retyui
Copy link
Contributor Author

retyui commented Sep 3, 2023

here is more deeper description #11528
but PR hasn't merged yet

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Hey! This issue is closed and isn't watched by the core team. You are welcome to discuss the issue with others in this thread, but if you think this issue is still valid and needs to be tracked, please open a new issue with a repro.

@osdnk
Copy link
Member

osdnk commented Sep 3, 2023

Hey, @retyui

I'm sorry I have to revert this PR #11576.

The concern, which I didn't notice earlier is that as some places (at example/src/Screens/NativeStackHeaderCustomization.tsx) you need to down-cast PlatformColor. Let me think about that.

It's actually quite common practice to use strings as colors and we don't want to break the code for our current users (it's small, but it's somehow breaking change).

I wonder, maybe we should introduce that together with some major as it's conceptually correct approach.

osdnk added a commit that referenced this pull request Sep 3, 2023
@retyui
Copy link
Contributor Author

retyui commented Sep 3, 2023

right in time when I did a tweet lol)

@osdnk you don't need to worry about example/ as theme isn't use a PlatformColor's there. (Otherwise, you must first resolve this problem: callstack/react-native-paper#4012)

@osdnk
Copy link
Member

osdnk commented Sep 3, 2023

Hi @retyui
I'm sorry for that, I can empathise it might be disappointing. Let me explain my reasoning here:

  1. It's not about, where is the bug, but more what's the final user experience. With this change in, we get a breaking change (with respect to typings).
  2. In react-navigation, we do color manipulation in many places to improve DX by automating some stuff (search for Color() and it only works with string. by supporting PlatformColor , those color manipulation are essentially pointless.

Is there a way we can support those kind of manipulations? Also, why do you need this change? Maybe understanding this motivation would help us to find out a good solution.

@khagesh
Copy link

khagesh commented Oct 4, 2023

From our use case, we are trying to use PlatformColor for switching between light & dark mode. PlatformColor is nice because we don't need to add if...else everywhere for theme and then set colors.

The way we can support the manipulation using Color is by checking PlatformColor type and then using resolveColorSync from @klarna/platform-colors. In this way, it won't be a breaking change, and we can have PlatformColor as well. But it does add dependency on @klarna/platform-colors, and also add checks for PlatformColor type inside react-navigation codebase. Personally, I don't think these are huge issues for react-navigation, but you would know better.

@khagesh
Copy link

khagesh commented Oct 4, 2023

I did this workaround to use PlatformColor with react-navigation theme. Basically, I did conversion to string before passing to NavigationContainer, and then made sure that NavigationContainer re-renders when color scheme on device changes using useColorScheme hook in App.tsx where we render NavigationContainer. Also, when scheme changes, we will get different values from PlatformColor, and hence new string values should be calculated.

import { resolveColorSync } from "@klarna/platform-colors";
......

  const scheme = useColorScheme();
  const theme = useMemo(() => {
    const navigationThemeColors = {
      primary: resolveColorSync(Colors.primary),
      background: resolveColorSync(Colors.background),
      card: resolveColorSync(Colors.background),
      text: resolveColorSync(Colors.primary),
      border: resolveColorSync(Colors.background),
      notification: resolveColorSync(Colors.primary),
    };
    return {
      ...(scheme === "dark" ? DarkTheme : DefaultTheme),
      colors: {
        ...(scheme === "dark" ? DarkTheme.colors : DefaultTheme.colors),
        ...navigationThemeColors,
      }
    };
  }, [scheme]);

...........

// pass theme 
<NavigationContainer theme={theme}>

Edit:
Maybe, we can add this somewhere in this page https://reactnavigation.org/docs/themes

@osdnk
Copy link
Member

osdnk commented Oct 5, 2023

Alternatively, if you don't rely on our colors' manipulations, you can simply ignore TS error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript Error: Type 'OpaqueColorValue' is not assignable to type 'string' when I use PlatformColor
4 participants