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

Introduce Compose Destinations for SDK Navigation #531

Draft
wants to merge 14 commits into
base: epic/sdk-navigation
Choose a base branch
from

Conversation

jumaallan
Copy link
Member

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

Summary

A few sentences/bullet points about the changes

Known Issues

Any shortcomings in your work. This may include corner cases not correctly handled or issues related
to but not within the scope of your PR. Design compromises should be discussed here if they were not
already discussed above.

Test Instructions

Concise test instructions on how to verify that your feature works as intended. This should include
changes to the development environment (if applicable) and all commands needed to run your work.

Screenshot

If applicable (e.g. UI changes), add screenshots to help explain your work.

@jumaallan jumaallan changed the base branch from main to epic/sdk-navigation January 21, 2025 11:14
@jumaallan jumaallan force-pushed the feat/refactor-ui-components branch from f5b930a to 60f92dd Compare January 22, 2025 18:08
Copy link
Contributor

@JNdhlovu JNdhlovu left a comment

Choose a reason for hiding this comment

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

Draft PR so I understand, left a few comments but selfie capture route is not working to the end video in slack

handle: SavedStateHandle,
): T = when (modelClass) {
OrchestratedSelfieViewModel::class.java -> OrchestratedSelfieViewModel(
navArgs = OrchestratedSelfieCaptureScreenDestination.argsFrom(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to use this for only selfie capture? if yes fine, if not maybe this needs to be generic or have some sort of reusablity

// jobId = jobId,
// allowAgentMode = allowAgentMode,
// showAttribution = showAttribution,
// showInstructions = showInstructions,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

Copy link
Contributor

@robin-ankele robin-ankele left a comment

Choose a reason for hiding this comment

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

The direction generally makes sense to me. As discussed yesterday, let's sync up with @tobitech to define the interfaces and then we can implement this product by product. Great work @jumaallan - thanks for exploring this option.

composeTestRule.onNodeWithText(subtitleText, substring = true).assertIsDisplayed()
composeTestRule.onNodeWithText(captureTitle, substring = true).assertIsDisplayed()
}
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Just putting a comment here so we don't forget to fix the tests before we release this

import com.smileidentity.util.randomJobId
import com.smileidentity.util.randomUserId
import com.smileidentity.viewmodel.BiometricKycViewModel
import com.smileidentity.viewmodel.viewModelFactory
import kotlinx.collections.immutable.ImmutableMap
import kotlinx.collections.immutable.persistentMapOf

@Destination<BiometricGraph>(start = true)
@Composable
internal fun OrchestratedSelfieCaptureScreengg() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this call the OrchestratedSelfieCaptureScreen?

Copy link
Contributor

@JNdhlovu JNdhlovu left a comment

Choose a reason for hiding this comment

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

Screen_Recording_20250219_121921_Smile.ID.mp4

Hey @jumaallan just a note on transitions and back press

Copy link

github-actions bot commented Mar 5, 2025

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants