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

Alert dialog to match the maximum of the width of its content #433

Merged
merged 3 commits into from
Mar 21, 2023

Conversation

m-sasha
Copy link
Member

@m-sasha m-sasha commented Mar 11, 2023

The problem here is the modifier.width(IntrinsicSize.Min) we pass to AlertDialogContent, asking it to size itself to the minimum width it can. This causes (at least) two problems:

  1. The dialog is trying to be as narrow as possible, not using the available horizontal space.
  2. Using Modifier.widthIn on the AlertDialog doesn't do anything, but it seems like a reasonable way to control the width of a dialog on the desktop.

Ideally, we shouldn't be trying to control the width at all (which would also get rid of the crash when you put lazy content in an AlertDialog), but the problem with that is with the fillMaxWidth() here:

        buttons = {
            // TODO: move the modifiers to FlowRow when it supports a modifier parameter
            Box(Modifier.fillMaxWidth().padding(all = 8.dp)) {
                AlertDialogFlowRow(
                    mainAxisSpacing = 8.dp,
                    crossAxisSpacing = 12.dp
                ) {
                    dismissButton?.invoke()
                    confirmButton()
                }
            }
        },

Without controlling the width outside of that, it will cause the dialog to always take the maximum available horizontal space.

But without the fillMaxWidth it's hard to position the AlertDialogFlowRow correctly because we can neither pass a modifier to AlertDialogFlowRow nor control its alignment inside the Column in AlertDialogContent.

Note that all the discussion below applies to the case when the user doesn't specify a width on the AlertDialog.

There are two possible solutions I see:

  1. Use IntrinsicSize.Max instead of IntrinsicSize.Min
  2. Try writing a custom layout to wrap AlertDialogFlowRow in, and don't specify any width on the AlertDialogContent.

Pros of 1:
The dialog would be sized to the content's maximum preferred width, but not wider. With the 2nd solution, it would always take the width of the entire window.

Pros of 2:
Using lazy content would not crash (although we said using lazy content in an AlertDialog is not a valid use-case). With the 1st solution, it would continue crashing.

Note that both solutions would change the existing (I would argue wrong) behavior which, if there are buttons present and the user didn't specify a width, would use the width of the buttons for the size of the dialog (because their minimum width is larger than the minimum width of the text). So before, it would look like this:

image

and after the change:

image

But note that this is only if the user doesn't specify a width (or widthIn) himself.

Proposed Changes

This PR goes with the simpler solution of using IntrinsicSize.Max. I think the benefit of sizing to the preferred content width is more important that allowing lazy content.

Testing

Test: Manual testing with code like this:

@OptIn(ExperimentalMaterialApi::class)
fun main() = singleWindowApplication(
    state = WindowState(width = 500.dp, height = 400.dp),
) {
    MaterialTheme{
        AlertDialog(
            modifier = Modifier.widthIn(max = 400.dp).padding(horizontal = 30.dp),
            onDismissRequest = { },
            title = { Text("This is the title") },
            text = { Text("This is the text This is the text This is the text") },
//            dismissButton = { Button(onClick = {}){ Text("Dismiss")} },
//            confirmButton = { Button(onClick = {}){ Text("Confirm")} },
            confirmButton = {}
        )
    }
}

Issues Fixed

Fixes: JetBrains/compose-multiplatform#2836

@m-sasha
Copy link
Member Author

m-sasha commented Mar 11, 2023

This PR is a lot of words for a 2-character change in the source :-)

@m-sasha m-sasha changed the title Changed the width of an alert dialog to match the maximum of the width of its content Alert dialog to match the maximum of the width of its content Mar 11, 2023
@adrientetar
Copy link

Maybe dialogs could have a default width set, and the developer can override that default if it doesn't work?

@dima-avdeev-jb
Copy link

@m-sasha Are you trying to run ./compose/scripts/testDesktop to check backward compatibility?

@m-sasha
Copy link
Member Author

m-sasha commented Mar 12, 2023

@m-sasha Are you trying to run ./compose/scripts/testDesktop to check backward compatibility?

I haven't, but I will now.

@m-sasha
Copy link
Member Author

m-sasha commented Mar 12, 2023

Maybe dialogs could have a default width set, and the developer can override that default if it doesn't work?

Maybe, but that requires a decision, and decisions are hard 😉

@m-sasha
Copy link
Member Author

m-sasha commented Mar 20, 2023

Added default padding of 24.dp to AlertDialog.

@m-sasha m-sasha requested a review from igordmn March 20, 2023 14:53
@m-sasha m-sasha force-pushed the alert-dialog-width-fix branch from 351359d to aa51fc1 Compare March 20, 2023 14:57
@m-sasha m-sasha force-pushed the alert-dialog-width-fix branch from aa51fc1 to 0b03916 Compare March 20, 2023 15:00
@m-sasha m-sasha merged commit fb5a763 into jb-main Mar 21, 2023
@m-sasha m-sasha deleted the alert-dialog-width-fix branch March 21, 2023 10:44
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.

AlertDialog is too narrow by default
4 participants