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

Use trait collection to resolve border colors #32492

Closed
wants to merge 1 commit into from

Conversation

danilobuerger
Copy link
Contributor

@danilobuerger danilobuerger commented Oct 28, 2021

Summary

c974cbf changed the border colors to be of UIColor instead of CGColor. This allowed working with dark mode to switch the border colors automatically. However, in certain situation the system can't resolve the current trait collection (see https://stackoverflow.com/a/57177411/2525941). This commit resolves the colors with the current trait collection to ensure the right colors are selected. This matches with the behavior of how the background color is resolved (also in displayLayer:).

Changelog

[iOS] [Fixed] - Resolve border platform color based on current trait collection

Test Plan

Same test plan as #29728

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 28, 2021
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Oct 28, 2021
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Oct 28, 2021
@pull-bot
Copy link

PR build artifact for 38d5f6b is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@analysis-bot
Copy link

analysis-bot commented Oct 28, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: f7a66e6
Branch: main

@analysis-bot
Copy link

analysis-bot commented Oct 28, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,297,788 -2,614
android hermes armeabi-v7a 7,634,879 -4,063
android hermes x86 8,771,104 -4,744
android hermes x86_64 8,708,122 -4,635
android jsc arm64-v8a 9,783,593 -1,242
android jsc armeabi-v7a 8,768,874 -1,847
android jsc x86 9,749,504 -1,956
android jsc x86_64 10,345,405 -2,222

Base commit: 384e1a0
Branch: main

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

For some reason CLANGFORMAT is flagging those lines:712, 730, 733

I'd say let's rebase this and verify if Circle CI is green. If is still green after the rebase and the internal job I can reformat it manually.

Also cc @sammy-SC for a iOS pass

facebook@c974cbf changed the border colors to be of UIColor instead of CGColor. This allowed working with dark mode to switch the border colors automatically. However, in certain situation the system can't resolve the current trait collection (see https://stackoverflow.com/a/57177411/2525941). This commit resolves the colors with the current trait collection to ensure the right colors are selected. This matches with the behavior of how the background color is resolved (also in displayLayer:).
@danilobuerger
Copy link
Contributor Author

Hi @cortinico thanks for the review. I rebased it and amended the lines you mentioned. It looks like there were some whitespaces that the linter didn't agree with.

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @danilobuerger in 9a35818.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 7, 2022
@danilobuerger
Copy link
Contributor Author

@cortinico Thank you for taking care of my outstanding PRs. I really do appreciate it.

@danilobuerger danilobuerger deleted the patch-4 branch February 7, 2022 18:41
@cortinico
Copy link
Contributor

@cortinico Thank you for taking care of my outstanding PRs. I really do appreciate it.

Sorry those fell through the cracks 👍 Glad we got to merge them though!

shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
Summary:
facebook@c974cbf changed the border colors to be of UIColor instead of CGColor. This allowed working with dark mode to switch the border colors automatically. However, in certain situation the system can't resolve the current trait collection (see https://stackoverflow.com/a/57177411/2525941). This commit resolves the colors with the current trait collection to ensure the right colors are selected. This matches with the behavior of how the background color is resolved (also in displayLayer:).

## Changelog

[iOS] [Fixed] - Resolve border platform color based on current trait collection

Pull Request resolved: facebook#32492

Test Plan: Same test plan as facebook#29728

Reviewed By: sammy-SC

Differential Revision: D33819225

Pulled By: cortinico

fbshipit-source-id: 2f8024be7ee7b32d1852373b47fa1437cc569391
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants