andrew-levy / sweet-sfsymbols

SF Symbols brought to you by Expo's Module API
65 stars 2 forks source link

`PlatformColor` is not supported as color value #5

Closed dlindenkreuz closed 9 months ago

dlindenkreuz commented 9 months ago

I am trying to use a PlatformColor to render an SF Symbol, but the icon is rendered as black:

<Icon
  name="minus.circle.fill"
  color={PlatformColor("systemRed")}
  scale="large"
/>

Looks like ExpoModulesCore does not support converting the opaque PlatformColor object to a UIColor. Does it make sense to add this logic to sweet-sfsymbols?

andrew-levy commented 9 months ago

Hey @dlindenkreuz that's a good question. I just looked at how PlatformColor works under the hood, and it looks like this should be added to expo modules core. PlatformColor("systemRed") resolves to a simple object {semantic: "systemRed"}, which is passed to that native side and eventually mapped to a UIColor. This should probably done during the converting phase. If it can't be added there, i'll add it here.

andrew-levy commented 9 months ago

specifically here https://github.com/expo/expo/blob/main/packages/expo-modules-core/ios/Core/Convertibles/Convertibles+Color.swift

dlindenkreuz commented 9 months ago

I am already working on a PR, will link it here when ready! 😄

dlindenkreuz commented 9 months ago

iOS support + tests are done, looking into Android now.

https://github.com/expo/expo/commit/b00b01658bb756a492f0896e83068eb6aaac65f3

dlindenkreuz commented 9 months ago

PR: https://github.com/expo/expo/pull/26724

Didn't figure out the Android part. Should be a fine improvement for iOS anyway.

andrew-levy commented 9 months ago

This is awesome! I was wondering if the Selector approach would work and it looks like it does. Nice job

dlindenkreuz commented 9 months ago

Thanks mate 🙏 the original implementation in RN uses selectors as well, apparently it's still the way to go in Swift for lack of a good Reflection feature 😉

andrew-levy commented 9 months ago

good to know! let me know if you need any help getting this PR merged