Skip to content

[WIP] Fix: Escape closes Settings dialog if login dialog open #3996 #4299

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

Draft
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

ssidharth010
Copy link

@ssidharth010 ssidharth010 commented Jun 29, 2025

Fix for: #3996

Problem:

The ESC button closes the dialog opened below rather than top.

Root Cause:

Since there is not close button configured on the login modal, but on the settings modal it have close button configuration.
Prime vue dialog takes only the one first one hence closing that dialog only.
primefaces/primevue#5138

Solution:

Current temporary solution is simply to add close button to Login modal.

Other solutions:

CloseOnEsc in the DialogStore config will always be kept false and a function can be written in the KeyBindingService to handle close on esc by closing only if the top most dialog has closable set to true

┆Issue is synchronized with this Notion page by Unito

@ssidharth010 ssidharth010 requested a review from a team as a code owner June 29, 2025 15:33
@ssidharth010 ssidharth010 marked this pull request as draft June 29, 2025 15:33
Copy link
Contributor

@webfiltered webfiltered left a comment

Choose a reason for hiding this comment

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

Currently, pressing escape closes both the user and settings dialogs.

Would it be possible to have escape close the user dialog, then close settings, to bring this in line with stacked dialogs elsewhere in the app?

@ssidharth010
Copy link
Author

ssidharth010 commented Jul 1, 2025

Currently from what i have checked with prime vue dialog doesn't have the option to configure that way.
The only options is to update our current flows to update the closeOnEsc to false for all the dialogs below the active dialog. This is the only work around that I can find at the moment.
This will also be a major change.

webfiltered and others added 17 commits July 1, 2025 09:46
Co-authored-by: webfiltered <[email protected]>
Co-authored-by: webfiltered <[email protected]>
@christian-byrne
Copy link
Contributor

The only options is to update our current flows to update the closeOnEsc to false for all the dialogs below the active dialog. This is the only work around that I can find at the moment.

In the dialogStore or dialogService, we can expose a function to close the "highest" dialog, while respecting the ranking system, then add the esc keyhandler to all dialogs that have close on escape prop enabled currently, then remove the close on escape prop. What do you think?

@ssidharth010
Copy link
Author

The only options is to update our current flows to update the closeOnEsc to false for all the dialogs below the active dialog. This is the only work around that I can find at the moment.

In the dialogStore or dialogService, we can expose a function to close the "highest" dialog, while respecting the ranking system, then add the esc keyhandler to all dialogs that have close on escape prop enabled currently, then remove the close on escape prop. What do you think?

Sure, i think that's the way to go.
I'll make the necessary changes.

@ssidharth010
Copy link
Author

@webfiltered I'll close this MR. Please continue the review on the below MR, I had some rebase issues with current branch.
#4364

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.

8 participants