SpeziOnboarding #129
SpeziOnboarding #129
Conversation
| @@ -0,0 +1,3 @@ | |||
| package edu.stanford.spezi.core.utils | |||
|
|
|||
| interface Standard | |||
There was a problem hiding this comment.
Let's please start creating correct modules as part of this PR already, you can use the script that I have shared. In iOS this is defined under Spezi package. Create a module under modules:spezi and define this interface there, and add modules:spezi dependency where you need this
There was a problem hiding this comment.
I would like to wait for this until SpeziAccount is merged, hope that's fine. Especially, I would want them to be self-contained (if this makes sense), i.e. to have no dependencies outside of their own package ecosystem and that will most likely mean that we will need to put some stuff into the packages that do not exist in the iOS version and that are Android-specific (e.g. SpeziTheme, etc).
There was a problem hiding this comment.
What I can do, is simply creating a single package for this interface, so that it isn't part of core:utils, if that's good enough
| @@ -34,25 +34,23 @@ internal fun SignaturePad( | |||
| val keyboardController = LocalSoftwareKeyboardController.current | |||
| Column { | |||
| OutlinedTextField( | |||
| value = uiState.firstName.value, | |||
| value = uiState.name.givenName ?: "", | |||
There was a problem hiding this comment.
Updating the usage to PersonNameComponents implies that we should at least make use of some of its capabilities, e.g. formatting, or usage of NameTextField. We are doing none of those here, and I personally see no need to replace.
I hope that you are also able to see some of the concerns that I raised in the SpeziViews PR in terms of usability of mutating the fields of person name components builder directly inside compose. Even if you would use a NameTextField, updates of the fields would not cause a recomposition of the SignatureCanvas for instance. The approach that we followed there just covers a single case of simply updating the fields, however real use cases as this one here, require updates of other components as well once for instance the name changed.
There was a problem hiding this comment.
Fine with reverting this to be two strings, instead? i.e. givenName and lastName - but only since they do not appear in the public API.
| Spacer(modifier = Modifier.height(Spacings.medium)) | ||
| Text("Signature:") | ||
| SignatureCanvas( | ||
| paths = uiState.paths.toMutableList(), | ||
| firstName = uiState.firstName.value, | ||
| lastName = uiState.lastName.value, | ||
| firstName = uiState.name.givenName ?: "", |
There was a problem hiding this comment.
This speaks for a downside of passing a mutable reference of the name components in NameTextField, updates of the fields would not cause a recomposition of the canvas
There was a problem hiding this comment.
see above - recomposition would work though, since the onAction updates the state with a new PersonNameComponents instance every time, since PersonNameComponents only contains constants.
| onAction(ConsentAction.Consent) | ||
| onAction(ConsentAction.Consent( | ||
| documentIdentifier = "consent", | ||
| exportConfiguration = ConsentDocumentExportConfiguration() |
There was a problem hiding this comment.
this is a business logic and should not get decided in the ui, please leave the consent action as it was and define the id and export configuration in the view model
There was a problem hiding this comment.
Let's talk about this, since it would make sense to stay aligned with the iOS interface here, and this information would need to be injected into the viewModel in some way
| import androidx.compose.ui.unit.dp | ||
|
|
||
| @Composable | ||
| fun OnboardingComposable( |
There was a problem hiding this comment.
There is no need to create so many wrapper layouts inside. We can add every content as an item in the lazy column:
@Composable
fun OnboardingComposable(
modifier: Modifier = Modifier,
title: @Composable () -> Unit = {},
content: @Composable () -> Unit,
action: (@Composable () -> Unit)? = null,
) {
LazyColumn(
modifier = modifier
.padding(Spacings.medium),
horizontalAlignment = Alignment.CenterHorizontally
) {
item { title() }
item { content() }
action?.let {
item {
VerticalSpacer(height = Spacings.small)
it()
}
}
}
}| import java.nio.charset.StandardCharsets | ||
|
|
||
| data class ConsentDocument( | ||
| private val markdown: suspend () -> ByteArray, |
There was a problem hiding this comment.
This is not being used. Please follow all the usages where we pass this and remove from the whole chain of calls
|
|
||
| import android.graphics.pdf.PdfDocument | ||
|
|
||
| class ConsentDocumentExport( |
There was a problem hiding this comment.
This is also just being created and not being used
| import edu.stanford.spezi.core.utils.UUID | ||
|
|
||
| class OnboardingNavigationPath internal constructor( | ||
| internal val navController: NavController, |
There was a problem hiding this comment.
this can be private, and other properties immutable vals
| fun append(id: String) { | ||
| val step = steps.firstOrNull { it.id == id } ?: error("") | ||
|
|
||
| navController.navigate( |
There was a problem hiding this comment.
why do we attempt to navigate when appending? This won't work if the route hasn't been registered to the graph first, same for the append method below
| } | ||
|
|
||
| @Composable | ||
| fun OnboardingStack( |
There was a problem hiding this comment.
Would propose the following here and usage of rembemberSaveable as that is what also rember nav controller useses:
@Composable
fun OnboardingStack(
modifier: Modifier = Modifier,
onComplete: () -> Unit = {},
startAtStep: String? = null,
content: OnboardingStackBuilder.() -> Unit,
) {
val navController = rememberNavController()
val navigationPath = rememberSaveable(navController, startAtStep, content) {
OnboardingNavigationPath(navController, startAtStep, content)
}
navigationPath.NavHost(modifier = modifier)
LaunchedEffect(navController) {
navController.currentBackStackEntryFlow.collect { entry ->
if (entry.destination.route == null) {
onComplete()
}
}
}
}As per navigation path:
class OnboardingNavigationPath internal constructor(
private val navController: NavHostController,
startDestination: String?,
builder: OnboardingStackBuilder.() -> Unit,
) {
private val steps = OnboardingStackBuilder().apply(builder).steps.also {
if (it.isEmpty()) error("No steps provided")
}
private val startDestination = startDestination ?: steps.first().id
// rest of the content
// nav host
@Composable
fun NavHost(modifier: Modifier) {
CompositionLocalProvider(LocalOnboardingNavigationPath provides this) {
NavHost(
modifier = modifier,
navController = navController,
startDestination = startDestination,
) {
composable(
route = customRoute("{id}"),
arguments = listOf(navArgument("id") { type = NavType.StringType })
) { backStackEntry ->
val step = backStackEntry.arguments?.getString("id")?.let { stepId ->
customSteps.firstOrNull { it.id == stepId }
}
step?.content?.let { it() } ?: IllegalOnboardingStep()
}
for (step in steps) {
composable(step.id) { step.content() }
}
}
}
}I just replicated your logic in the custom route, but I personally see that logic not being needed
SpeziOnboarding
♻️ Current situation & Problem
Link any open issues or pull requests (PRs) related to this PR. Please ensure that all non-trivial PRs are first tracked and discussed in an existing GitHub issue or discussion.
⚙️ Release Notes
Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Include code snippets that provide examples of the feature implemented or links to the documentation if it appends or changes the public interface.
📚 Documentation
Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.
✅ Testing
Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: