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

fix: improved file dialog consistency across OSes #2971

Merged
merged 1 commit into from
May 20, 2022

Conversation

@Skaiir Skaiir requested review from barmac and MaxTru May 18, 2022 14:41
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label May 18, 2022
@github-actions
Copy link

This Pull Request targets develop branch, but contains fix commits.

Consider targeting master instead.

@Skaiir Skaiir changed the base branch from develop to master May 19, 2022 07:58
@barmac
Copy link
Collaborator

barmac commented May 19, 2022

I like the new primary button look but the buttons have wrong order on Mac (tested on the linked artifact):
image

@Skaiir
Copy link
Contributor Author

Skaiir commented May 19, 2022

@barmac Every time I start thinking I understand how mac does this, I get fooled. What does the dialog look like when you close a modified diagram (changes not saved)

@barmac
Copy link
Collaborator

barmac commented May 19, 2022

image

@barmac
Copy link
Collaborator

barmac commented May 19, 2022

I'll check on my machine how the dialog template looks like in the debugger.

@barmac
Copy link
Collaborator

barmac commented May 19, 2022

Incredible. It looks OK in the debugger :D
image

@barmac
Copy link
Collaborator

barmac commented May 19, 2022

As I found out, the way MacOS handles this is expected. It's part of the Human Interface Guidelines from Apple.

Cf. electron/electron#23276 (comment) and https://developer.apple.com/design/human-interface-guidelines/macos/windows-and-views/alerts/

Place buttons where people expect them. In general, the button people are most likely to choose should be on the right. The default button should always be on the right. Cancel buttons are typically on the left.

So ✅ from me, nothing to change for Mac specifically.

@MaxTru
Copy link
Contributor

MaxTru commented May 20, 2022

Arch Linux with KDE:
Open Empty
Screenshot_20220520_091832

  • Please note that even though not visually indicated, the Create is the default option (e.g., just pressing Enter will create the file)
  • This is unchanged from the existing implementation if I am not mistaken

Save edited
Screenshot_20220520_091924

  • Please note that here Cancel (the middle one) is the default option. <=== this appears to be strange to me. Why is cancel in the middle?
  • The order is changed from the existing implementation. See how we do it currently:
    Screenshot_20220520_092100

@Skaiir
Copy link
Contributor Author

Skaiir commented May 20, 2022

@MaxTru This order thing is confusing but this is actually how it should be, see the screenshot of vscode you sent me last month.

image

However save should be focused, that's a mistake on my part. 😄

The ordering already exists in the codebase, it was done in this ticket: #2300

@Skaiir Skaiir force-pushed the 2970-empty-file-dialog-os-consistency branch from b8ec0da to 31a0a82 Compare May 20, 2022 12:56
@MaxTru
Copy link
Contributor

MaxTru commented May 20, 2022

Works on Linux now (ordering as in my previous comment, but now save is focussed) .

LGTM ✔️

@barmac
Copy link
Collaborator

barmac commented May 20, 2022

LGTM as well. As a follow-up, we could also improve the activiti conversion dialog:
image

so that it looks like DMN 1.1 dialog:
image

@Skaiir
Copy link
Contributor Author

Skaiir commented May 20, 2022

@barmac Unfortunately that's not exactly possible. If there's only two buttons it'll be horizontally stacked, the only reason the DMN one is vertical is the checkbox 😆

Unless you mean we should also include the checkbox.

@Skaiir Skaiir merged commit 6b405f2 into master May 20, 2022
@Skaiir Skaiir deleted the 2970-empty-file-dialog-os-consistency branch May 20, 2022 13:32
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 20, 2022
@barmac
Copy link
Collaborator

barmac commented May 20, 2022

This, and also use the defaultId property.

@Skaiir Skaiir mentioned this pull request Jul 8, 2022
40 tasks
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.

Make empty file dialog buttons intuitive across all supported OS
3 participants