-
Notifications
You must be signed in to change notification settings - Fork 987
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
chore: resolve some deprecation warnings #21874
base: develop
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (26)
|
bfdace4
to
3edfa00
Compare
3edfa00
to
06085c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've flagged some commit in my commit history with the flag: prefix. This is meant to describe that I'm not sure if the commit causes a regression. For example, in some cases we've relied on colors/custom-color for choosing a specific color based on the theme of the app, but sometimes we've overridden this pattern, so replacing these usages with colors/resolve-color is not 100% compliant. So perhaps we should review these changes carefully and decide whether we need to still use custom-colour for these use-cases.
@seanstrom Are you suggesting that colors/custom-color
might have legitimate use cases that resolve-color
can't directly address? If so, should we consider removing the deprecation warning from custom-color
?
@ilmotta yes, I think For example, it seems that
So perhaps we should create some new functions that handle these two use-cases separately, one function for overriding colours ( Thoughts? |
@seanstrom The idea of using explicit color functions for particular use cases is a nice idea. It appears that |
5c97827
to
0c38f2e
Compare
0c38f2e
to
1322742
Compare
Okay cool, I've added a new function named
Ahh good catch! I've replaced that usage with |
50% of end-end tests have passed
Failed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (2)Click to expandClass TestWalletMultipleDevice:
Passed tests (4)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
1322742
to
87fe66f
Compare
50% of end-end tests have passed
Failed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestFallbackMultipleDevice:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (28)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestFallbackMultipleDevice:
|
relates to #21868
Summary
colors/custom-color
andrn/touchable-without-feedback
.colors/custom-color
have been replaced withcolors/resolve-color
, but some usages remain because we still rely on some of the special behaviour ofcolors/custom-color
when retrieving the color for a network.rn/touchable-without-feedback
have been replaced withrn/pressable
.quo/text-combinations
Side Note
flag:
prefix. This is meant to describe that I'm not sure if the commit causes a regression. For example, in some cases we've relied oncolors/custom-color
for choosing a specific color based on the theme of the app, but sometimes we've overridden this pattern, so replacing these usages withcolors/resolve-color
is not 100% compliant. So perhaps we should review these changes carefully and decide whether we need to still usecustom-colour
for these use-cases.Platforms
Areas that maybe impacted
Potentially everything
status: ready