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

feat: replace native confirmation popups with antd popconfirms #1611

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

eglitise
Copy link
Collaborator

This PR is a style change for the confirmation dialogs that appear when deleting a saved capability set or gesture.

  • The current implementation uses the native browser alert/Electron full window popup, which blocks all interaction with the application until either of the two prompt options are selected.
  • The proposed implementation switches to the antd Popconfirm component, which offers several benefits:
    • More lightweight UX
    • Consistent style with the rest of the app
    • Automatically dismissed upon any action outside the dialog
    • i18n support

Screenshot 2024-08-16 at 16 32 28

Note: antd does not auto-hide tooltips on click, so for saved capability sets, the zIndex prop was used to place the tooltip behind the popconfirm.

@github-actions github-actions bot added the enhancement New feature or request label Aug 16, 2024
@mykola-mokhnach
Copy link
Contributor

Do we only want to replace a single session deletion confirmation popup in this PR or there are more?

@eglitise
Copy link
Collaborator Author

These are the only two instances of window.confirm in the Inspector code.

@eglitise
Copy link
Collaborator Author

Both changes are applied to every element within their lists, so this will replace all instances of the deletion confirmation popups.

@mykola-mokhnach
Copy link
Contributor

I mean maybe there are other types of alerts, for example informational ones, and we'd like to replace them as well

@eglitise
Copy link
Collaborator Author

True, we do use notification in various places, but antd also offers message, which may be more useful in some locations. That can come in a separate PR though 🙂

@eglitise eglitise merged commit e11b40a into appium:main Aug 16, 2024
6 checks passed
@eglitise eglitise deleted the replace-fullscreen-confirms branch August 16, 2024 19:40
laib3 pushed a commit to laib3/appium-inspector that referenced this pull request Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants