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.

Sign up for GitHub

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

Jump to bottom

[modules-core] add convertible support for PlatformColor and DynamicColorIOS #26724

Merged
merged 6 commits into from
Feb 1, 2024
Merged

[modules-core] add convertible support for PlatformColor and DynamicColorIOS #26724

merged 6 commits into from
Feb 1, 2024

Conversation

dlindenkreuz
Copy link
Contributor

Why

PlatformColor and DynamicColorIOS are currently not supported by expo-modules-core.

Packages based on Expo Modules should be able to use colors that are resolved at runtime, allowing for Dark Mode adjustments and system colors. React Native has supported this since a while upstream and it's time to bring this to Expo Modules as well.

Important: This PR only adds support for iOS. It looks like ColorPropConverter needs to be called from Expo but I have no experience with the Android platform.

How

  • Add implementation to Convertibles+Color.swift
  • Implementation mimics original React Native RCTConvert implementation
  • Notable differences from original implementation:
    • No fallback colors (originally added to support iOS <13.0) are required, since Expo only supports iOS >=13.2 as of now
    • UIColor class vars like UIColor.systemRed never return an array in Swift → no NSArray handler required

Test Plan

  • Added unit tests to ConvertiblesSpec.swift

Checklist

@dlindenkreuz dlindenkreuz mentioned this pull request Jan 26, 2024
Closed
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jan 26, 2024
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jan 26, 2024
@dlindenkreuz
Copy link
Contributor Author

Looks like the CI is green except for these two steps:

Both fail with:

An Expo user account is required to proceed.
Either log in with eas login or set the EXPO_TOKEN environment variable if you're using EAS CLI on CI (Learn more: https://docs.expo.dev/accounts/programmatic-access/)

@tsapeta does this need a fix?

@tsapeta
Copy link
Member

tsapeta commented Jan 29, 2024

Hey @dlindenkreuz! Thanks so much for submitting such a great PR! I'll review it today.

You can ignore these CI jobs, looks like an issue on our side because forks have no access to repo secrets - to be honest we shouldn't even run them on external contributions 🤔

tsapeta
tsapeta requested changes Jan 31, 2024
View reviewed changes
Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

Generally looks good, I left just a few nitpicks 😄

packages/expo-modules-core/ios/Core/Convertibles/Convertibles+Color.swift Outdated Show resolved Hide resolved
packages/expo-modules-core/ios/Core/Convertibles/Convertibles+Color.swift Outdated Show resolved Hide resolved
packages/expo-modules-core/ios/Core/Convertibles/Convertibles+Color.swift Outdated Show resolved Hide resolved
packages/expo-modules-core/ios/Core/Convertibles/Convertibles+Color.swift Show resolved Hide resolved
packages/expo-modules-core/ios/Core/Convertibles/Convertibles+Color.swift Outdated Show resolved Hide resolved
@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Jan 31, 2024
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jan 31, 2024
tsapeta
tsapeta approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

Nice, thanks again for this improvement! Are you interested in implementing this for Android too? We can give you some guidance on it 😉

@tsapeta tsapeta changed the title [expo-modules-core] add convertible support for PlatformColor and DynamicColorIOS [modules-core] add convertible support for PlatformColor and DynamicColorIOS Feb 1, 2024
@tsapeta tsapeta merged commit f8f7eb7 into expo:main Feb 1, 2024
8 of 11 checks passed
@dlindenkreuz dlindenkreuz deleted the fix-color-convertibles branch February 2, 2024 12:45
@dlindenkreuz
Copy link
Contributor Author

Phew, I have never built anything for Android before so I have no idea about the platform…

At the latest, I'll get back to this when my project reaches the Android part 😜

@brentvatne brentvatne added the published Changes from the PR have been published to npm label Feb 16, 2024
@dlindenkreuz
Copy link
Contributor Author

@brentvatne I am using expo@50.0.11 and expo-modules-core@1.11.10 and it seems like the change from this PR doesn't exist anymore even though it's merged to main. 🤯

How can I use this in my project?

tsapeta pushed a commit that referenced this pull request Mar 13, 2024
@dlindenkreuz @tsapeta
[modules-core] add convertible support for PlatformColor and `Dynam…
b7bb768 
…icColorIOS` (#26724)
@tsapeta
Copy link
Member

tsapeta commented Mar 13, 2024

Hi @dlindenkreuz,

This PR was merged after the SDK 50 was released, that is after the release branch was cut off, so normally it would be released in SDK 51. main branch has things that are to be released in SDK 51 and the code that is used in SDK 50 is on the sdk-50 branch.

We usually don't backport new features to already released SDKs, but since this change seems to be safe to release with SDK 50, I just cherry-picked your commit to that branch, so it will be published in expo@50.0.13, hopefully in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsapeta tsapeta tsapeta approved these changes

@lukmccall lukmccall lukmccall approved these changes

@Kudo Kudo Awaiting requested review from Kudo Kudo is a code owner

Assignees
No one assigned
Labels
bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dlindenkreuz @tsapeta @lukmccall @brentvatne @expo-bot