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

Add Sheet confirmValueChange callback #45

Closed
TheKeeperOfPie opened this issue Nov 26, 2024 · 11 comments
Closed

Add Sheet confirmValueChange callback #45

TheKeeperOfPie opened this issue Nov 26, 2024 · 11 comments
Assignees
Labels
feature request New feature or request

Comments

@TheKeeperOfPie
Copy link

Wiring this up manually ended up being more complicated than I expected, and I can't get a working modal.

Essentially I have a persistent M3 BottomSheetScaffold right now which is a form that the user submits. If the user makes a change and then swipes the sheet away, I use M3's confirmValueChange to block the swipe down* and prompt them with a dialog to either confirm the exit or save the edits.

I can't find an equivalent in Composables Core. It seems difficult to block SheetDetents.Hidden, which I think I can approximate by swapping the detents list, removing/adding .Hidden as necessarily to disable the swipe down. But I actually don't want to block the swipe itself. If a user initiates a swipe, I want it to succeed and only prompt them after, so that swiping down is still a valid indication from the user to exit the form.

*Note that this is currently broken in M3 from what I understand due to a bug, which is what caused me to look for alternatives and end up here. I have an extremely hacky solution right now observing the expand/hide requests and manually reversing them, but the UX isn't great.

@alexstyl
Copy link
Member

There is no similar functionality but it seems like a valid request.

Let me have a look and I will let you know ASAP.

@alexstyl alexstyl added the feature request New feature or request label Nov 26, 2024
@alexstyl alexstyl changed the title Does BottomSheet have an equivalent for confirmValueChange? Support Sheet confirmValueChange Nov 26, 2024
@alexstyl alexstyl changed the title Support Sheet confirmValueChange Add Sheet confirmValueChange callback Nov 26, 2024
@alexstyl
Copy link
Member

PS: If you can share code of how you expect this to work, it would help tons.

@TheKeeperOfPie
Copy link
Author

I'm not totally sure, because the M3 APIs don't support this exactly either, but what I want is roughly:

@Stable
class State {
    var formField by mutableStateOf("")

    var showExitDialog by mutableStateOf(false)
    var confirmedExit by mutableStateOf(false)
    
    val showEvents = MutableSharedFlow<Boolean>()

    fun hasChanged() = formField != ""

    suspend fun show() { showEvents.emit(true) }
    suspend fun hide() { showEvents.emit(false) }
}

@Composable
fun FormBottomSheetScaffold(state: State) {
    Scaffold(...) {
        val sheetState = rememberBottomSheetState(
            initialDetent = SheetDetents.Hidden,
            detents = listOf(SheetDetents.Hidden, SheetDetents.FullyExpanded),
        )
        LaunchedEffect(Unit) {
            state.showEvents.collectLatest {
                if (it) {
                    sheetState.animateTo(SheetDetents.FullyExpanded)
                    state.confirmedExit = false
                } else {
                    sheetState.animateTo(SheetDetents.Hidden)
                }
            }
        }
        BottomSheet(
            state = sheetState,
            confirmValueChange = {
                if (it == SheetDetents.FullyExpanded) {
                    true
                } else if (state.hasChanged()) {
                    state.showExitDialog = true
                    false
                } else {
                    true
                }
            }
        ) {
            TextField(value = state.formField, onValueChanged = { state.formField = it })
        }

        if (state.showExitDialog) {
            AlertDialog(
                ...,
                onConfirm = {
                    state.confirmedExit = true
                    state.showExitDialog = false
                },
                onDismiss = { state.hide() },
            )
        }
    }
}

@alexstyl
Copy link
Member

What you shared is what I had in mind you needed 👍

M3 APIs don't support this exactly

what do you mean? It seems like the M3 api. is it different?

@TheKeeperOfPie
Copy link
Author

what do you mean? It seems like the M3 api. is it different?

M3's current confirmValueChange in my example gets called whenever the TextField changes (more specifically I have a button which increments a field), this causes state.showExitDialog = true to be called even if the user hasn't swipe dismissed the sheet, interrupting them while editing.

It's possible I'm holding it wrong and somewhere in my hierarchy I have a recomposition or something which causes it to be called incorrectly. I've never bothered to dig into it, I just know the behavior broke several months ago so I hacked something together and ignored it for now.

@alexstyl alexstyl transferred this issue from composablehorizons/composables Nov 27, 2024
@alaegin
Copy link

alaegin commented Nov 30, 2024

This is a highly awaited feature for our app.
We must prevent BottomSheet from being closed while making some stuff (requests, audio recording, etc.)

@alexstyl
Copy link
Member

alexstyl commented Dec 1, 2024

Working on this today.

It is a bit more complex than expected because I think I bumped into the behavior @TheKeeperOfPie describes on the Material Compose one.

Working on making the API and callbacks make sense, so you don't spend hours debugging it like the Material Compose one. I think it is doable tho

@alexstyl
Copy link
Member

alexstyl commented Dec 1, 2024

Made Bottom Sheet's work nicely and predictably 👍
Working on Modal Sheet now

@alexstyl alexstyl self-assigned this Dec 1, 2024
@TheKeeperOfPie
Copy link
Author

Oops, hopefully I caught you early enough, but this behavior works when using the M3 modal sheet. I actually recently migrated mine to modal just to have my tests work correctly. I haven't tried to do it using Composables Core, but with M3, this works:

@Stable
class State {
    var formField by mutableStateOf("")

    var showExitDialog by mutableStateOf(false)
    var confirmedExit by mutableStateOf(false)
    
    var showing by mutableStateOf(false)

    fun hasChanged() = formField != ""

    fun show() {
        showing = true
        confirmedExit = false
    }
}

@Composable
fun FormBottomSheetScaffold(state: State) {
    Scaffold(...) {
        val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true)
        val scope = rememberCoroutineScope()
        if (state.showing) {
            ModalBottomSheet(
                sheetState = sheetState,
                onDismissRequest = {
                    if (state.hasChanged() && !state.confirmedExit) {
                        state.showExitDialog = true
                        scope.launch { sheetState.expand() }
                    } else {
                        state.showing = false
                    }
                },
            ) {
                TextField(value = state.formField, onValueChanged = { state.formField = it })
            }
        }

        if (state.showExitDialog) {
            AlertDialog(
                ...,
                onConfirm = {
                    state.hasConfirmedClose = true
                    state.showExitDialog = false
                    scope.launch { sheetState.hide() }
                },
            )
        }
    }
}

But to be clear, there are distinct differences between the modal and scaffold approach, and I would really want the scaffold to work. It allows you to have the sheet content always in the composition tree, and it doesn't block interacting with the app content like a dialog does.

@alexstyl
Copy link
Member

alexstyl commented Dec 1, 2024

Done. Will be released in the next version of the library

alexstyl added a commit that referenced this issue Dec 1, 2024
alexstyl added a commit that referenced this issue Dec 1, 2024
The `confirmDetentChange()` allows you to block the dismissal of the sheet depending on a state.

Fixes: #45
@alexstyl
Copy link
Member

alexstyl commented Dec 2, 2024

This is now available in 1.20.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants