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

FR: Dialog: Actions & Keybindings #131

Closed
henry2man opened this issue Jan 12, 2022 · 8 comments · Fixed by #156
Closed

FR: Dialog: Actions & Keybindings #131

henry2man opened this issue Jan 12, 2022 · 8 comments · Fixed by #156
Labels
blocked Issue blocked by another issue or pull-request enhancement New feature or request

Comments

@henry2man
Copy link
Contributor

I think the dialogues can be improved a bit.

After working with other frameworks and component palettes I think we could have the following improvements.

  • Actions: Instead of having a wide-open "List <Widget>? actions" we could have some specific class like DialogAction that allows having a title, maybe an (optional) icon, an onTap / onClick callback and more importantly, attributes like isDefaultAction or isDestructiveAction. The presentation would be as is usually done now with Button elements but we would already have semantics and the ability to work better with the UI (the default action is highlighted in color, the destructive action is red, RTL support ...)

  • Related to the above, I miss some key combinations:

    • Esc to close the active dialog
    • Enter to "execute the isDefaultAction=true action callback"
    • Tab to rotate the focus between the available actions, always starting with the default action

What do you think?

@bdlukaa
Copy link
Owner

bdlukaa commented Jan 12, 2022

Esc should already pop modal routes in flutter (showDialog, for example). I don't know why it sometimes doesn't work

Enter should execute the focused button when pressed but, for some reason, the buttons aren't getting focus when the dialog shows up. Same for Tab

I don't like the idea of DialogAction, since in flutter anything can be a widget. Maybe the dev wants custom buttons and it wouldn't be possible with it

@bdlukaa bdlukaa added the enhancement New feature or request label Jan 12, 2022
@henry2man
Copy link
Contributor Author

I don't like the idea of DialogAction, since in flutter anything can be a widget. Maybe the dev wants custom buttons and it wouldn't be possible with it

Well, maybe I didn't explain the proposal well. I'm not saying that DialogAction shouldn't be a Widget. In fact, the equivalent in Cupertino Widgets does is a Widget:

https://api.flutter.dev/flutter/cupertino/CupertinoDialogAction-class.html

https://github.com/flutter/flutter/blob/ab0a33597328772fe27dbc0b217ac930373ef5f0/packages/flutter/lib/src/cupertino/dialog.dart#L1691

@bdlukaa
Copy link
Owner

bdlukaa commented Jan 12, 2022

But, in Windows, normal buttons are used as actions

@henry2man
Copy link
Contributor Author

henry2man commented Jan 12, 2022

But, in Windows, normal buttons are used as actions

I agree. I don't want to have something different in terms of UI and end-user visible results. As I said, we could implement the UI of DialogActions using plain Button elements but with some superpowers, like Cupertino Widgets does.

My proposal simply wants to improve the developer experience & end user usability, that's all 👍

@bdlukaa
Copy link
Owner

bdlukaa commented Feb 1, 2022

Keybindings

About ESC to close the dialog, see flutter/flutter#97581. Native behavior is to not allow user to tap ouside, that's why barrierDismissible is false, but Flutter also isn't allowing us to close the dialog pressing ESC. For a current workaround, set barrierDismissible: true

I tested with the latest stable (2.8.1) and pressing Tab to navigate through the actions works, as well as pressing Enter to execute their action. For default action, you can use autofocus on a button

@bdlukaa bdlukaa added the blocked Issue blocked by another issue or pull-request label Feb 1, 2022
@henry2man
Copy link
Contributor Author

@bdlukaa On Esc this could be not obvious but, after thinking on it this behavior probably makes sense.

And the use of autofocus could make the trick.

So, with current widgets we could be able to replicate most of my requirements in this FR. Nevertheless it could be interesting consider the implementation of a specialized DialogAction widget.

@bdlukaa
Copy link
Owner

bdlukaa commented Feb 2, 2022

Hello! Now, on the latest master, is possible to close the dialog opened by showDialog pressing Esc. I used CallbackShortcuts + FocusScope to detect the key events. This is a workaround while flutter/flutter#97581 isn't fixed on the framework!

@bdlukaa bdlukaa mentioned this issue Feb 5, 2022
6 tasks
@bdlukaa
Copy link
Owner

bdlukaa commented Feb 5, 2022

Hello! Now the dialog can be closed with Esc properly.

Also, for the buttons, I think we can stay with the current buttons. FilledButton for primary button and Button for other actions!

Repository owner locked and limited conversation to collaborators Feb 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked Issue blocked by another issue or pull-request enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants