Add Missing Navigation Buttons #302

tobitech · 2025-03-06T13:58:47Z

Story: https://app.shortcut.com/smileid/story/11514

Summary

Improve the BiometricKYC Flow to allow users return to ID Information collection flow.

Known Issues

N/A.

Test Instructions

Run a Biometric KYC job.
When you land on the Selfie Capture screen you should be able to cancel selfie capture and return to ID information collection. (This is so that users can return if they accidentally proceeded from ID information screen).
Partner developers should also be able to implement their own dismiss functionality when a user cancels from selfie capture with the onDismiss closure.

Screenshot

N/A

…oup.

…ic and enhanced.

github-actions · 2025-03-06T13:59:37Z

	Warnings
⚠️	The source files were changed, but the tests remain unmodified. Consider updating or adding to the tests to match the source changes.
⚠️	Example/SmileID/BiometricKYC/BiometricKycWithIdInputScreenViewModel.swift#L107 - Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (`colon`)
⚠️	Sources/SmileID/Classes/BiometricKYC/OrchestratedBiometricKycViewModel.swift#L157 - TODOs should be resolved (- Fix when Michael changes thi...). (`todo`)
⚠️	Sources/SmileID/Classes/SmileID.swift#L161 - Function should have complexity 10 or less: currently complexity equals 11 (`cyclomatic_complexity`)
⚠️	Sources/SmileID/Classes/SmileID.swift#L188 - TODOs should be resolved (- Fix when Michael changes thi...). (`todo`)

Generated by 🚫 Danger Swift against 5c803cd

…tion.

robin-smileid

Tested a few flows with biometric kyc and cancel sends the user back to the id document capture. Great work @tobitech .

jumaallan · 2025-03-12T08:14:08Z

Example/SmileID/BiometricKYC/BiometricKycWithIdInputScreen.swift

+                    ActivityIndicator(isAnimating: true).padding()
+                    Text(SmileIDResourcesHelper.localizedString(for: messageKey))
+                        .font(SmileID.theme.body)
+                        .foregroundColor(SmileID.theme.onLight)


should we replace this with the .light option or this does not get affected by dark mode on the device

This is from the theme class. Not affected by dark mode on the device.
PS: This is the same as on the main branch. I only wrapped the body of the screen inside a ZStack to prevent glitches.

jumaallan · 2025-03-12T08:15:35Z

Example/SmileID/Helpers/CancellableNavigationView.swift

+                            onCancel()
+                        } label: {
+                            Text(SmileIDResourcesHelper.localizedString(for: "Action.Cancel"))
+                                .foregroundColor(SmileID.theme.accent)


same question about this

jumaallan · 2025-03-12T08:18:28Z

Sources/SmileID/Classes/BiometricKYC/OrchestratedBiometricKycViewModel.swift

@@ -38,7 +33,7 @@ class OrchestratedBiometricKycViewModel: ObservableObject {
        if selfieFile != nil {
            submitJob()
        } else {
-            updateStep(.selfie)
+            updateProcessingState(nil)


We should maybe have a state that is not nil

Like use an actual name so it's even easier to go through the code and understand

The state is used to drive navigation to the ProcessingScreen. nil means the job submission hasn't started, so processing screen isn't showing. That's why it's an optional value.

but we could have a default value right which'd make it more consistent or there is a con to that?

jumaallan · 2025-03-12T08:19:04Z

Sources/SmileID/Classes/BiometricKYC/OrchestratedBiometricKycViewModel.swift

@@ -103,7 +98,7 @@ class OrchestratedBiometricKycViewModel: ObservableObject {

        guard selfieFile != nil else {
            // Set step to .selfieCapture so that the Retry button goes back to this step
-            updateStep(.selfie)
+            updateProcessingState(nil)


jumaallan · 2025-03-12T08:19:46Z

Sources/SmileID/Classes/Navigation/CancellableNavigationView.swift

+                        onCancel()
+                    } label: {
+                        Text(SmileIDResourcesHelper.localizedString(for: "Action.Cancel"))
+                            .foregroundColor(SmileID.theme.accent)


theme question here since this is inside the sdk

jumaallan · 2025-03-12T08:22:34Z

Sources/SmileID/Classes/SmileID.swift

    public class func biometricKycScreen(
        config: BiometricVerificationConfig,
-        delegate: BiometricKycResultDelegate
+        delegate: BiometricKycResultDelegate,
+        onDismiss: (() -> Void)? = nil


we don't expose this on android - not opposed to it, but maybe we need to add something like that on android so that the wrappers are in sync

We also use the delegate to send the user cancelled action since you can use the back buttons on android

Yeah, basically this is an orchestrated flow. We discussed that we put a constraint on how we want orchestrated flows to be presented. The goal was for orchestrated flows to only be presented as an interstitials and can be dismissed so that users can return back to the original flow.
This closure is for developers to handle that dismiss if they want to... notice that it's optional.
The other goal here is to separate navigation actions from job submission events (didSucceed or didError).

We can discuss about unifying the SDK methods.

How would this be better than a delegate/onResult with user cancelled as we have now?

jumaallan

Left some comments

JNdhlovu · 2025-03-18T12:45:36Z

Sources/SmileID/Classes/SmileID.swift

    public class func biometricKycScreen(
        config: BiometricVerificationConfig,
-        delegate: BiometricKycResultDelegate
+        delegate: BiometricKycResultDelegate,
+        onDismiss: (() -> Void)? = nil


How would this be better than a delegate/onResult with user cancelled as we have now?

JNdhlovu · 2025-03-18T12:53:50Z

Sources/SmileID/Classes/SelfieCapture/View/OrchestratedEnhancedSelfieCaptureScreen.swift

+                        }
+                    )
+                    .transition(.move(edge: .leading))
+                } else {


Can we not have this condition just be determined by state instead of a variable and also state?

JNdhlovu · 2025-03-18T12:56:34Z

Sources/SmileID/Classes/BiometricKYC/OrchestratedBiometricKycViewModel.swift

@@ -38,7 +33,7 @@ class OrchestratedBiometricKycViewModel: ObservableObject {
        if selfieFile != nil {
            submitJob()
        } else {
-            updateStep(.selfie)
+            updateProcessingState(nil)


but we could have a default value right which'd make it more consistent or there is a con to that?

tobitech added 4 commits March 6, 2025 12:38

rename product container view and move to a new file under helpers gr…

ae9b998

…oup.

make the SmartSelfieAuthWithUserIdentry view reusuable for both class…

3009acb

…ic and enhanced.

implement dismiss for orchestrated enhance selfie capture

adfa255

embed biometric kyc views in a zstack.

dd0023a

tobitech self-assigned this Mar 6, 2025

improve biometric kyc flow so that users can return to id info collec…

38b9096

…tion.

tobitech marked this pull request as ready for review March 10, 2025 08:59

tobitech requested a review from a team as a code owner March 10, 2025 08:59

code formatting.

5c803cd

robin-smileid approved these changes Mar 10, 2025

View reviewed changes

jumaallan reviewed Mar 12, 2025

View reviewed changes

tobitech requested a review from jumaallan March 14, 2025 15:50

JNdhlovu requested changes Mar 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Missing Navigation Buttons #302

Add Missing Navigation Buttons #302

tobitech commented Mar 6, 2025 •

edited

Loading

github-actions bot commented Mar 6, 2025 •

edited

Loading

robin-smileid left a comment

jumaallan Mar 12, 2025

tobitech Mar 12, 2025

jumaallan Mar 12, 2025

jumaallan Mar 12, 2025

tobitech Mar 12, 2025

JNdhlovu Mar 18, 2025

jumaallan Mar 12, 2025

jumaallan Mar 12, 2025

jumaallan Mar 12, 2025

tobitech Mar 12, 2025 •

edited

Loading

JNdhlovu Mar 18, 2025

jumaallan left a comment

JNdhlovu Mar 18, 2025

JNdhlovu Mar 18, 2025

JNdhlovu Mar 18, 2025

Add Missing Navigation Buttons #302

Are you sure you want to change the base?

Add Missing Navigation Buttons #302

Conversation

tobitech commented Mar 6, 2025 • edited Loading

Summary

Known Issues

Test Instructions

Screenshot

github-actions bot commented Mar 6, 2025 • edited Loading

robin-smileid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobitech Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumaallan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobitech commented Mar 6, 2025 •

edited

Loading

github-actions bot commented Mar 6, 2025 •

edited

Loading

tobitech Mar 12, 2025 •

edited

Loading