Add beta macOS implementation to experimental-avatar to test arm64 pipeline #577
Conversation
Saadnajmi
changed the title
| override func constantsToExport() -> [AnyHashable : Any]! { | ||
| return [ | ||
| "sizes" : [ | ||
| "xSmall" : 16, | ||
| "small" : 24, | ||
| "medium" : 32, | ||
| "large" : 40, | ||
| "xLarge" : 52, | ||
| "xxLarge" : 72 | ||
| ] | ||
| ] | ||
| } |
There was a problem hiding this comment.
API difference:
iOS has these explicit sizes you can set the Avatar too. macOS supports any size. To keep the same API, I just hardcoded sizes for macOS avatar to use and call the constructor with the appropriate CGFloat,
There was a problem hiding this comment.
yeah, can we bring this up with the design team? Avatar for mac is still bit undefine but there are lots of churn i don't know Taili's team might have more guidance :)
There was a problem hiding this comment.
There's a figma somewhere with values I haven't looked up.. but that would be a FluentUI Apple change ideally. The purpose of this was really to unblock arm64 testing, so would it be cool to keep this enum here (perhaps with better defined constants) and integrate it back into FUA with the better design once we have it?
There was a problem hiding this comment.
I asked Kaan to start, and he said the point values will most likely stay the same. Nevertheless, I filed microsoft/fluentui-apple#391 to track adding this to FluentUI Apple. I'll update that in code
| RCT_REMAP_VIEW_PROPERTY(imageSource, contactImage, UIImage) | ||
|
|
||
| RCT_REMAP_VIEW_PROPERTY(backgroundColor, avatarBackgroundColor, UIColor) |
There was a problem hiding this comment.
Fun fact: even though this is macOS, you need to set these as UIImage and UIColor for it to work properly
There was a problem hiding this comment.
can we add comments on this? i cringe a little
There was a problem hiding this comment.
Yes.. though lots of controls will have things that maps to colors and images (like in Amanda's change). Is it still worth adding a comment if I have to add it everywhere?
There was a problem hiding this comment.
Does this just work because UIImage and UIColor are macros defined to the correct platform types?
There was a problem hiding this comment.
If so, react-native-macos defines various RCT* wrappers around these native types that we could try using instead
There was a problem hiding this comment.
As in, specify something like RCTUIColor instead? I'll try that.
It seems that UIColor and UIImage map to their corresponding RCTConvert macros, which contain the correct macOS implementation. Both and and @chiuam had problems setting it to just NSColor.
There was a problem hiding this comment.
Yea we need to change that at some point, but probably fine for now as that's out of scope for this review
|
its interesting that mac avatar background color doesn't match with iOS. i thought we updated the mac avatar background palettes to be the same? |
The palettes are the same, but the hashing algorithm is different. When we did the work to integrate Avatar for it's first client (ToDo) it was requested but ultimately cut. So nothing should block us from "fixing" the hash algorithm to match. |
| xcode_configuration: 'Debug' | ||
| xcode_extraArgs: '-xcconfig $(Build.Repository.LocalPath)/.ado/xcconfig/publish_overrides.xcconfig' | ||
| xcode_extraArgs: '-xcconfig $(Build.Repository.LocalPath)/.ado/xcconfig/publish_overrides.xcconfig' ONLY_ACTIVE_ARCH=NO |
There was a problem hiding this comment.
So... this totally works, but It would be cleaner to not need to manually add the xcconfig here. I'll follow up with the react-native-test-app team on getting the change integrated upstream
| RCT_REMAP_VIEW_PROPERTY(imageSource, contactImage, UIImage) | ||
|
|
||
| RCT_REMAP_VIEW_PROPERTY(backgroundColor, avatarBackgroundColor, UIColor) |
There was a problem hiding this comment.
Yea we need to change that at some point, but probably fine for now as that's out of scope for this review
|
FYI @harrieshin, I filed microsoft/fluentui-apple#392 for the color mismatch issue |
Platforms Impacted
Description of changes
I need a native module on macOS to test that the static libraries are indeed universal and compiled for x86 and arm64.
Since @chiuam is already working on button, I decided to add a macOS implementation to Avatar...
Note: I don't consider this macOS avatar implementation production ready, and have marked it "Beta" as such. There's a few API differences / hardcoded constants and such that we would need to recitfy. If you'll believe me, I REALLY did add it so I can test the e2e macOS pipeline for arm64.
This change adds a macOS (native) implementation for
experimental-avatar, and packages the compiled static library into our nuget package. We also fix a bug on the iOS implementation where a custom backgroundColor was not getting set properly.Verification
Ran both iOS and macOS test apps to make sure Avatar displayed properly.
Pull request checklist
This PR has considered (when applicable):