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

Smart Selfie Capture Flow #497

Merged
merged 42 commits into from
Dec 13, 2024
Merged

Smart Selfie Capture Flow #497

merged 42 commits into from
Dec 13, 2024

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 requested a review from a team as a code owner November 19, 2024 08:48
@jumaallan jumaallan changed the title Smart Selfie Flow Smart Selfie Capture Flow Nov 19, 2024
@prfectionist
Copy link

prfectionist bot commented Nov 19, 2024

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Hardcoded Value
The progress animation uses a hardcoded target value of 0.5F which may not reflect actual progress. Consider making this dynamic based on actual capture progress.

Code Removal
Significant UI components were removed (DirectiveVisual, AnimatedImageFromSelfieHint) without clear replacement for visual feedback during selfie capture process

Code Duplication
Commented out code block is kept alongside new implementation. Should be removed if no longer needed.

@prfectionist
Copy link

prfectionist bot commented Nov 19, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Enhancement
Add duration parameter to animation specification for smoother visual transitions

The animation duration for progress indicator is missing. Add a duration parameter
to the tween animation spec for smoother animation.

lib/src/main/java/com/smileidentity/compose/selfie/v2/SelfieCaptureScreenV2.kt [279]

-animationSpec = tween(easing = LinearEasing),
+animationSpec = tween(durationMillis = 1000, easing = LinearEasing),
Suggestion importance[1-10]: 6

Why: Adding an explicit animation duration improves the user experience by ensuring consistent and smooth animations. This is a functional improvement that affects the UI behavior.

6
Maintainability
Split chained modifiers into multiple lines for better code readability

The chained modifiers should be on separate lines for better readability and
maintainability. Split the chained modifiers into multiple lines.

lib/src/main/java/com/smileidentity/compose/selfie/FaceShapedProgressIndicator.kt [47]

-Canvas(modifier = modifier.progressSemantics(progress).fillMaxSize()) {
+Canvas(
+    modifier = modifier
+        .progressSemantics(progress)
+        .fillMaxSize()
+) {
Suggestion importance[1-10]: 4

Why: The suggestion improves code readability by breaking down chained modifiers into separate lines, making the code more maintainable and easier to modify. However, it's a minor stylistic improvement.

4
Organize constructor parameters in a more logical order with core parameters first

The SelfieCaptureParams constructor call has many parameters that could be more
readable using named arguments.

lib/src/main/java/com/smileidentity/compose/SmileIDExt.kt [85-95]

 val commonParams = SelfieCaptureParams(
     userId = userId,
     jobId = jobId,
+    isEnroll = true,
     allowNewEnroll = allowNewEnroll,
     allowAgentMode = allowAgentMode,
     showAttribution = showAttribution,
     showInstructions = showInstructions,
     extraPartnerParams = extraPartnerParams,
-    isEnroll = true,
     skipApiSubmission = skipApiSubmission,
 )
Suggestion importance[1-10]: 3

Why: While the suggestion improves code organization by grouping core parameters first, the existing code is already using named arguments and is quite readable. The improvement is minimal and purely cosmetic.

3

@jumaallan jumaallan temporarily deployed to Play Store Internal App Sharing December 3, 2024 09:22 — with GitHub Actions Inactive
@jumaallan jumaallan deployed to Play Store Internal App Sharing December 9, 2024 19:36 — with GitHub Actions Active
allowNewEnroll: Boolean = false,
showAttribution: Boolean = true,
showInstructions: Boolean = true,
useStrictMode: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Needed still?

showInstructions = showInstructions,
isEnroll = true,
showAttribution = showAttribution,
useStrictMode = useStrictMode,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@@ -182,6 +220,37 @@ fun SmileID.SmartSelfieAuthentication(
)
}

@Composable
Copy link
Contributor

Choose a reason for hiding this comment

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

ios sitll has this within the same product but this as an option

resultCallbacks.selfieViewModel?.let {
SelfieCaptureScreen(
// SelfieCaptureScreen(
// modifier = modifier,
Copy link
Contributor

Choose a reason for hiding this comment

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

new destination maybe to not break this?

@jumaallan jumaallan merged commit a3d8b81 into feat/new-ui-epic Dec 13, 2024
1 of 2 checks passed
@jumaallan jumaallan deleted the feat/new-capture-ui branch December 13, 2024 15:31
jumaallan added a commit that referenced this pull request Dec 13, 2024
* New UI Instructions Screen (#449)

* setting up paparazzi

* setting up v3 instruction screen

* added selfie capture screen ui and tests

* add v2 UI

* bump up VERSION (#455)

* Bump the androidx group with 5 updates (#454)

* Bump the agp group with 2 updates (#453)

* add instruction screen to new ui

* duplicate theme and cleaning up

* updated selfie capture flow

* updated tests

---------

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix Insecure Object Serialization (#456)

* fix unsecure object serialization

* updated CHANGELOG.md

* updated CHANGELOG.md

* bump up version (#457)

* Bump peter-evans/create-pull-request in the github-actions group (#458)

* bump up agp version (#459)

* Bump com.slack.lint.compose:compose-lint-checks in the all group (#461)

* Bump the androidx group with 5 updates (#460)

* Bump the kotlin group with 5 updates (#462)

* Bump the all group with 4 updates (#464)

* feat: scale document bitmaps based on available memory (#465)

* feat: scale document bitmaps based on available memory

* chore: fix docv tests

* feat: refactor on capture get memory first

* chore: undo test crashlytics

* fix: throw oom error when encountered

* chore: update changelog

* Update CHANGELOG.md

Co-authored-by: Ed Fricker <[email protected]>

---------

Co-authored-by: Ed Fricker <[email protected]>

* chore: bump sdk version to 10.3.3 (#467)

* Bump io.github.ujizin:camposer from 0.4.1 to 0.4.2 in the all group (#470)

* Bump the androidx group with 3 updates (#469)

* Bump the agp group with 2 updates (#468)

* Inflow Navigation (#401)

* feat:sdk-navigation

* feat: type safe nav wip

* fix: undo tests renaming

* feat: custom nav types

* feat: working nav

* feat:replace composables for enrollment with graph nav

* feat: main graph and reusable composble for nav

* feat: bump compose nav versions

* feat: refactor with seperation between orchestrated and indvidual screens

* fix: tests

* chore: bump compose navigation version

* feat: docs

* feat: main nav fixes

* feat: nav complete

* feat: nav complete

* feat: nav complete

* fix: unit tests for doc v and enhanced doc v

* feat: callbacks fixes

* feat: compose stack fixes and retries

* feat: pr feedback fixes

* feat: merge main

* feat: fix transition issue delay

* fix: nav cancelled actions

* feat: pop transitions

* feat: pop transitions on orch nav

---------

Co-authored-by: Juma Allan <[email protected]>

* feat: bump version to 10.3.3 (#473)

* feat: bump version to 10.3.3

* feat: changelog format

* feat: bump snapshot version (#474)

* Bump the all group with 3 updates (#472)

* Bump org.jetbrains.kotlinx:kotlinx-serialization-json in the all group (#477)

* Bump the agp group with 2 updates (#475)

* Bump com.google.devtools.ksp in the kotlin group (#471)

* Bump the androidx group with 8 updates (#476)

* feat: skip api submission (#478)

* prep: release 10.3.4 (#479)

* Smart Selfie Capture Flow (#497)

* reworking smart selfie flow

* code formatting and linting

* updated capture flow to show animations

* mark submitJob as non suspend

* reworking directives and lottie animations

* reworking shape indicator v2

* updated lottie animations

* cleaned up animations and capture ui

* updated hints to show different states

* cleaning up progress updates

* cleaning up animations

* updated haptic feedback

* updated the progress to make it more natural

* cleaning up the background and flaky animations

* updated naming

* updated the animations lottie files

* fixed the progress bars not filling up well

* fixed portrait lock on image

* make progress update faster and fixed orientation lottie

* updated the force brightness composable to keep the screen on

* added metadata and updated strings

* revert nav changes

* bump up to 10.4.0

* updated extension class and theming

* updated changelog

* updated ktlint version

* disable linting temporarily

* disable linting temporarily

* cleaning up

* fixed broken tests

* update preview

* fixed device spec

* added cancel button

* fixed inconsistent document type setup

* updated changelog

* renamed enhanced view model

* cleaning up

* update camera metadata

* updated build configs

* updated ktlint setup

* updated ktlint setup

* disable ktlint

* fixed lint issue

* fixed lint issues

* updated changelog

---------

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: JNdhlovu <[email protected]>
Co-authored-by: Ed Fricker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants