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

Escape works as "go back" in settings #4205

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maxphilippov
Copy link
Collaborator

@maxphilippov maxphilippov commented Oct 10, 2024

Did this as a test, cause it seemed like an easy refactor. And it was

Works, Notifications do look kinda weird.
image

There's an explicit "close settings" call when we click on "Add Second Device". @r10s @nicodh Should that remain or no? Here's a video of how it works

20241011_014510.mp4

Depends on #4006 so I'm not asking for review right now, too many changes displayed here

@maxphilippov maxphilippov changed the title Maxph/3868 escape in settings Escape works as "go back" in settings Oct 10, 2024
@r10s
Copy link
Member

r10s commented Oct 15, 2024

works nicely! also the stacking is much more what one would expect.

the PR needs a rebase or so, cannot say anything to the code by the thousands of changed lines :)

There's an explicit "close settings" call when we click on "Add Second Device". @r10s @nicodh Should that remain or no? Here's a video of how it works

if possible, it would nice if cancelling the initial "Add Second Device" alert is also shown atop of the settings dialog and canceling it just gets you back to settings. this would back trying out things and "clicking around, discovering what is possible" more predictable.

once the "Add Second Device" is started via "Continue", however, the settings dialog should disapepar so that one is full into the context of the "Add Second Device".

but that is no biggie - if it is hard to do, it is also fine as it is now.

@r10s
Copy link
Member

r10s commented Oct 15, 2024

as i was asked: for the "X" button, indeed it would be a bit weird if it closes all dialogs if they are visually stacked. this is not what is done usually in this case.

so either:

  • for the stacking, the "back" button is superfluous, remove it and use only "X"
  • do not stack visually, let "back" button close one level, "X" the whole dialog. "Escape" acts as the "back" button

i would go with the second option, to not add additional noise to UI, also it is what user already learned in Delta Chat :)

Copy link
Contributor

@nicodh nicodh left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Maybe the remaining focus border after closing a child dialog with ESC key could also be removed?

image

@nicodh
Copy link
Contributor

nicodh commented Oct 15, 2024

Works, Notifications do look kinda weird.

what do you mean?

@maxphilippov
Copy link
Collaborator Author

maxphilippov commented Oct 15, 2024

Works, Notifications do look kinda weird.

what do you mean?

Just not used to it I guess, the fact that I see Settings in the background and Notifications are much smaller. Nothing wrong with dialog contents

@maxphilippov
Copy link
Collaborator Author

if possible, it would nice if cancelling the initial "Add Second Device" alert is also shown atop of the settings dialog and canceling it just gets you back to settings.

Rebased, added that. But it doesn't close when we press Continue, doesn't bother me, if it's not critical I'd merge. I'm looking into the whole "close everything in stack when we confirm some action" question as part of #2268

@maxphilippov maxphilippov marked this pull request as ready for review October 15, 2024 21:51
@Simon-Laux
Copy link
Member

similar issue exists on pressing escape when making a new profile, there it should never close the dialog completely

@Simon-Laux
Copy link
Member

as i was asked: for the "X" button, indeed it would be a bit weird if it closes all dialogs if they are visually stacked. this is not what is done usually in this case.

so either:

  • for the stacking, the "back" button is superfluous, remove it and use only "X"
  • do not stack visually, let "back" button close one level, "X" the whole dialog. "Escape" acts as the "back" button

i would go with the second option, to not add additional noise to UI, also it is what user already learned in Delta Chat :)

I would like to have this question from r10s addressed before we merge it. I would personally also prefer the second option.

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.

4 participants