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-2542 Ds/insurances #2289

Merged
merged 3 commits into from
Nov 15, 2024
Merged

GEN-2542 Ds/insurances #2289

merged 3 commits into from
Nov 15, 2024

Conversation

StylianosGakis
Copy link
Member

I changed one thing which might affect other screens which was the default padding around the entire bottom sheet.
With previous implementation, if we passed nothing in the sheetPadding value it would also move the sheet up above the bottom navigation bar, which is never what we want to do anyway.
This might have affected other callers as well, but I will try to go through them to make sure they look OK

@StylianosGakis StylianosGakis requested a review from a team as a code owner November 15, 2024 09:51
LazyColumn(
contentPadding = WindowInsets
.safeDrawing
.only(WindowInsetsSides.Horizontal + WindowInsetsSides.Bottom)
.exclude(consumedWindowInsets)
Copy link
Contributor

@panasetskaya panasetskaya Nov 15, 2024

Choose a reason for hiding this comment

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

what was the problem with the insets here? was it orientation change? or tab changing?

},
) {
if (hedvigBottomSheetState.data != null) {
content(hedvigBottomSheetState.data!!)
Copy link
Contributor

@panasetskaya panasetskaya Nov 15, 2024

Choose a reason for hiding this comment

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

why do we need data here? in other words, I prob don't fully understand the usage of bottomSheetState :)

modifier = Modifier.placeholder(
visible = isLoading,
highlight = PlaceholderHighlight.shimmer(),
shape = MaterialTheme.shapes.squircleLarge,
shape = HedvigTheme.shapes.cornerXLarge,
Copy link
Contributor

@panasetskaya panasetskaya Nov 15, 2024

Choose a reason for hiding this comment

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

maybe diff corner here? placeholder look quite different from the button

@panasetskaya panasetskaya changed the title Ds/insurances GEN-2542 Ds/insurances Nov 15, 2024
Copy link

feature-insurances

@StylianosGakis StylianosGakis merged commit 2b2023f into develop Nov 15, 2024
3 of 4 checks passed
@StylianosGakis StylianosGakis deleted the ds/insurances branch November 15, 2024 15:38
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