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

GEN-3226 Feature/travel addon deeplink #2414

Merged
merged 7 commits into from
Feb 12, 2025

Conversation

panasetskaya
Copy link
Contributor

@panasetskaya panasetskaya commented Feb 6, 2025

  • need to add a new key slack_link at TravelAddonTriageDestination.kt.118

not sure about navigateUp vs popBackStack here

@panasetskaya panasetskaya marked this pull request as ready for review February 6, 2025 14:46
@panasetskaya panasetskaya requested a review from a team as a code owner February 6, 2025 14:46
@panasetskaya panasetskaya changed the title Feature/travel addon deeplink GEN-3226 Feature/travel addon deeplink Feb 10, 2025
Copy link

Copy link
Member

@StylianosGakis StylianosGakis left a comment

Choose a reason for hiding this comment

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

A note about navigateUp and popBackStack here where we need to call a different function.
And about always emitting some Ui from the screen.
All good otherwise 😊

Comment on lines 77 to 81
LaunchedEffect(uiState.insuranceIds) {
val params = uiState.insuranceIds
launchFlow(params)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

In the success case here we'd be emitting no UI at all.
Without knowing how this looks like, often this has led to animation glitches in navigation since we go from taking up 0x0 size to taking up the full size, and this size change is done with an animation by expanding the content from the center or top-left which is not what we want.

Can we either add an empty Box() in here, or even better wrap the entire when statement here in a Surface(color = ...background, modifier = Modifier.fillMaxSize()) { when(uiState)... } so that we always do emit at least the background properly?

HedvigErrorSection(
onButtonClick = onNavigateToNewConversation,
subTitle = stringResource(R.string.GENERAL_ERROR_BODY),
// todo: another key here! ask Richard
Copy link
Member

Choose a reason for hiding this comment

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

Is this decided by now?

Spacer(Modifier.weight(1f))
HedvigTextButton(
stringResource(R.string.general_close_button),
onClick = dropUnlessResumed { popBackStack() },
Copy link
Member

Choose a reason for hiding this comment

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

So from a button which normally takes you back one item in the backstack, but it is not the "up" arrow on the top-left of the screen, then what we must do is call this function here

override fun popBackStack() {
if (!navController.popBackStack()) {
updatedFinishApp()
}
}

Easiest to pass down the popBackStack function from this navigator
val navigator: Navigator = rememberNavigator(hedvigAppState.navController, finishApp)
down to this screen, and call that one on back from this button.
Since this screen does not seem to have a top app bar which we'd call navigateUp() from the top-left arrow, we probably just don't need to pass it down here at all in the first place.

@panasetskaya panasetskaya merged commit 52273dc into develop Feb 12, 2025
4 checks passed
@panasetskaya panasetskaya deleted the feature/travel-addon-deeplink branch February 12, 2025 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants