-
Notifications
You must be signed in to change notification settings - Fork 9
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 Latest TCA #575
Use Latest TCA #575
Conversation
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.
Lets have Umair test a PreAlpha release to assert this does not introduce any unknown new bugs.
@@ -1672,6 +1672,7 @@ | |||
isa = XCBuildConfiguration; | |||
baseConfigurationReference = 833DA0912A0B7BCF004D0A4D /* iOS-preAlpha.xcconfig */; | |||
buildSettings = { | |||
IPHONEOS_DEPLOYMENT_TARGET = 16.4; |
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.
Version should be set by updating the config
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.
Have we made Matt/Russ aware up bumping min deployment target??
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.
Matt suggested it. Perhaps you weren't actually there that day?
} | ||
.presentationDragIndicator(.visible) | ||
.presentationDetentIntrinsicHeight() | ||
.presentationDetents([.height(.smallDetent)]) |
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.
Isn't it better to use .fraction
to allow dynamic height based on the screen size? Or we do really want to enforce a given height?
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 don't have any preference, but the old version made the height fixed so that's what I did. I can test with fraction, at the risk of things looking a bit too airy on a large phone.
@@ -23,7 +22,7 @@ struct Completion: Sendable, FeatureReducer { | |||
|
|||
func reduce(into state: inout State, viewAction: ViewAction) -> EffectTask<Action> { | |||
switch viewAction { | |||
case .closeButtonTapped, .willDisappear: | |||
case .closeButtonTapped: |
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.
So .willDissapear
is not needed anymore? I think it was used for the case when the user is dismissing the view by swiping.
@@ -67,7 +66,7 @@ public struct SubmitTransaction: Sendable, FeatureReducer { | |||
} | |||
)) | |||
} | |||
case .willDisappear, .closeButtonTapped: | |||
case .closeButtonTapped: |
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.
Same here, will the .dismiss
event be sent to the delegate when the view is dismissed by swiping.
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.
LGTM overall, but the iOS needs to be updated differently.
Tickets
ABW-1736
ABW-1710
Description
This PR upgrades to using the latest version of TCA, meaning hundreds of commits of bug fixes and small improvements.
Generally speaking I think we should consider refactoring away our use of Relay, since this is what forces us to use a forked version of TCA, in making it much less automatic to keep up to date with TCA.
Notes
The version use here is not the 1.0 prerelease, meaning it does not use the updated type names like Reducer and Effect, but it does contain the final version of the navigation tools.
How to test
Since TCA is used on every single screen, the app should be thoroughly tested. At the same time, the upgrade is not expected to change any behaviour anywhere, so in all likelihood nothing will have changed.
Also fixes Authorized dApps
When disconnecting the last persona from a dApp, the dApp itself should be forgotten, and its screen popped. Before this PR, the dApp was indeed forgotten, but the screen was never popped from the stack.
Before:
https://github.com/radixdlt/babylon-wallet-ios/assets/123396602/6d0f0609-686a-4b82-ba7d-29e4b26d5e35
After:
https://github.com/radixdlt/babylon-wallet-ios/assets/123396602/34d38ad9-2c81-4959-99c4-5987337ff910
PR submission checklist