-
Notifications
You must be signed in to change notification settings - Fork 29
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
Sign-In through android and desktop webview #90
Sign-In through android and desktop webview #90
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.
Overall looks good! Just did a quick first pass to get you some early feedback
...rc/commonMain/kotlin/social/androiddev/signedout/navigation/DefaultSignedOutRootComponent.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/social/androiddev/signedout/navigation/DefaultSignedOutRootComponent.kt
Show resolved
Hide resolved
...rc/commonMain/kotlin/social/androiddev/signedout/navigation/DefaultSignedOutRootComponent.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/social/androiddev/signedout/navigation/DefaultSignedOutRootComponent.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/social/androiddev/signedout/navigation/DefaultSignedOutRootComponent.kt
Outdated
Show resolved
Hide resolved
ui/common/src/androidMain/kotlin/social/androiddev/common/composables/webview/SignInWebview.kt
Outdated
Show resolved
Hide resolved
ui/common/src/androidMain/kotlin/social/androiddev/common/composables/webview/SignInWebview.kt
Outdated
Show resolved
Hide resolved
ui/common/src/androidMain/kotlin/social/androiddev/common/composables/webview/SignInWebview.kt
Outdated
Show resolved
Hide resolved
ui/common/src/androidMain/kotlin/social/androiddev/common/composables/webview/SignInWebview.kt
Outdated
Show resolved
Hide resolved
ui/common/src/androidMain/kotlin/social/androiddev/common/composables/webview/SignInWebview.kt
Outdated
Show resolved
Hide resolved
Thank you @crocsandcoffee for your feedback which I have taken into account. Tell me if I can switch the pr to non-draft |
8e7fbe2
to
6faf9e3
Compare
ui/common/src/commonMain/kotlin/social/androiddev/common/composables/webview/SignInViewModel.kt
Outdated
Show resolved
Hide resolved
ui/common/src/commonMain/kotlin/social/androiddev/common/composables/webview/SignInWebview.kt
Outdated
Show resolved
Hide resolved
ui/signed-out/src/commonMain/kotlin/social/androiddev/signedout/composables/SignInContent.kt
Outdated
Show resolved
Hide resolved
ui/signed-out/src/commonMain/kotlin/social/androiddev/signedout/composables/SignInContent.kt
Outdated
Show resolved
Hide resolved
@crocsandcoffee for your information, I moved the sign-in webview, from the common module to the signed-out module because its implementation is specific to the sign-in use case. I changed the structure of the packages in the signed-out module to have a package-by-step (landing, sign-in). I would like to know what you think about it. |
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.
Looks good, let's go ahead and move this out of draft
ui/signed-out/src/androidMain/kotlin/social/androiddev/signedout/signin/SignInWebView.kt
Show resolved
Hide resolved
ui/signed-out/src/androidMain/kotlin/social/androiddev/signedout/signin/SignInWebView.kt
Outdated
Show resolved
Hide resolved
ui/signed-out/src/desktopMain/kotlin/social/androiddev/signedout/signin/SingInWebView.kt
Show resolved
Hide resolved
ui/signed-out/src/desktopMain/kotlin/social/androiddev/signedout/signin/SingInWebView.kt
Outdated
Show resolved
Hide resolved
ui/signed-out/src/commonMain/kotlin/social/androiddev/signedout/signin/SignInContent.kt
Show resolved
Hide resolved
Yes, I missed that in the first pass of the draft review, it definitely makes sense to move it from common to signed-out module. We can of course create a more general WebView composable in the common module and have the "SignInWebView" call that if we want. Also the package structure in the signed-out module sounds good |
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.
Great work 👏
e64ea72
to
a716381
Compare
📑 What does this PR do?
The login is done through a Webview. So I set up two implementations of the Webview, one on Android and the second on desktop.
The pr aims to identify a viable solution to use a Webview on a desktop. For this, I used JavaFX without going through the official plugin because it is not currently possible to use it in the module alongside the Android Library plugin.
✅ Checklist
🧪 How can this PR been tested?
🧾 Tasks Remaining: (List of tasks remaining to be implemented)
🖼️ Screenshots (if applicable):
Screen.Recording.2022-12-04.at.02.25.03.mov