oleksandrbalan / modalsheet

Modal Sheet library for Jetpack Compose
Apache License 2.0
121 stars 6 forks source link

[ModalSheet] Add data based ModalSheet for dynamic content #2

Closed oleksandrbalan closed 2 years ago

oleksandrbalan commented 2 years ago

Should fix #1

Do not really like the API though :/

@wooodenleg What do you think?

ModalSheet(
    data = error,
    visible = { this != null },
    onDismiss = onDismiss
) { data ->
    if (data == null) {
        return@ModalSheet
    }
    // Use `data` for content
}

https://user-images.githubusercontent.com/20944869/167252435-6a31b477-d1a1-40bb-b6fb-9c14f65aee05.mp4

filwiesner commented 2 years ago

I think this makes it to have duplicit information. What is the use case to have separate isVisible and data parameters? I imagined API like this:

ModalSheet(
    data: T?,
    onDismiss: () -> Unit,
    content: @Composable (T) -> Unit,
) { /* ... */ }

That means that we can close the sheet while displaying the old "data". Nullability becomes the visibility

filwiesner commented 2 years ago

I belive MVP could be as easy as this:

@Composable
fun <T> ModalSheet(
    data: T?,
    onDismiss: () -> Unit,
    content: @Composable (T) -> Unit,
) {
    var lastNonNullData by remember { mutableStateOf(data) }
    DisposableEffect(data) {
        if (data != null) lastNonNullData = data
        onDispose {}
    }

    ModalSheet(
        visible = data != null,
        onDismiss = onDismiss
    ) {
        lastNonNullData?.let {
            content(it)
        }
    }
}

The problem is how to behave when the transition is not T -> null but T -> T'

oleksandrbalan commented 2 years ago

The problem is how to behave when the transition is not T -> null but T -> T'

Then there will be no animation 🤷‍♂️ At least for now

https://user-images.githubusercontent.com/20944869/167285578-32c62a04-1d72-4194-91e9-e9fefa7a22cf.mp4

filwiesner commented 2 years ago

Then there will be no animation 🤷‍♂️ At least for now

Yeah, I would love to have something like AnimatedContent in the future which would hide the sheet and show the new data. But I would agree that's and edge case and should be an future improvement rather than main feature