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

v0.63 merge TODOs #621

Closed
20 of 25 tasks
alloy opened this issue Sep 29, 2020 · 3 comments
Closed
20 of 25 tasks

v0.63 merge TODOs #621

alloy opened this issue Sep 29, 2020 · 3 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@alloy
Copy link
Member

alloy commented Sep 29, 2020

For once #613 is merged.

Not now

  • Should we run clang-format as per upstream? facebook@d0871d0

    • Should probably do this last so we can easily merge in master
    • Ensure it also runs on cpp files
    • Add a git/CI hook to ensure files are prettified?
    • Currently upstream master is not formatted either, so doesn't seem like the right time. Asking upstream maintainers what their plans are.
  • Libraries/PushNotificationIOS/RCTPushNotificationManager.mm:
    old deprecated macOS ifdef'ed code replaced by: RCTPromiseResolveValueForUNNotificationSettings

  • Libraries/PushNotificationIOS/RCTPushNotificationManager.mm:
    use UUUserNotificationCenter on macOS in removeAllDeliveredNotifications, removeDeliveredNotifications, getDeliveredNotifications

@ghost ghost added the Needs: Triage 🔍 label Sep 29, 2020
@alloy alloy mentioned this issue Sep 29, 2020
9 tasks
@alloy
Copy link
Member Author

alloy commented Oct 2, 2020

@tom-un Do you recall why you didn’t move these iOS changes upstream in your (dynamic) color changes?

@tom-un
Copy link
Collaborator

tom-un commented Oct 4, 2020

It was per this code review feedback from nscoding: facebook#27908 (comment)

And resulted in this commit by me: tom-un@7a4629b

In short: in the react-native-macos fork, and in my initial PR upstream, in -[RCTView displayLayer:] needed to get a "flattened" color via _backgroundColor.CGColor using the correct traits. In react-native-macos it was doing a [UITraitCollection setCurrentTraitCollection:[self traitCollection]] , call _backgroundColor.CGColor, then restore UITraitCollection via [UITraitCollection setCurrentTraitCollection:savedTraitCollection]. Nscodings suggestion was to use [_backgroundColor resolvedColorWithTraitCollection:self.traitCollection].CGColor instead so that's what's upstream.

@alloy
Copy link
Member Author

alloy commented Oct 5, 2020

@tom-un Ok, so if I understand correctly we now have a iOS specific solution upstream and still need the platform guard in our fork with your previous change? It sounds like the upstream version should just use the change you previously had and then we can get rid of the change in our fork, no? Scratch that, I lost track of your previous change also being an iOS only change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants