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 (AIQ-4414): w/ add stopPropagation to prevent closing modal if Esc pressed inside MultiSelect #149

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

Conversation

GlebShchipanov
Copy link
Contributor

It looks for me that this stopPropagation wouldnt break anything, but if anyone have any concerns - please comment

Also i found that case MenuActions.CloseSelect right above my changes never fired, anyone know anything about this? Maybe we can remove that excessive part?

Copy link
Member

@cafloyd cafloyd left a comment

Choose a reason for hiding this comment

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

Tested on Storybook and confirmed that this change doesn't seem to negatively impact any existing functionality.

stopPropagation can often have unintended consequences, but this seems like an example of a case where it is appropriate to use.

I'm going to approve, but @GlebShchipanov please wait a day or so to merge to give others an opportunity to provide feedback.

@cafloyd
Copy link
Member

cafloyd commented Nov 14, 2024

Also i found that case MenuActions.CloseSelect right above my changes never fired, anyone know anything about this? Maybe we can remove that excessive part?

From the code, it looks like CloseSelect applies when the user presses 'alt' and the arrow up key - from source/shared.ts:

} else if (key === Keys.Up && altKey) {
			return MenuActions.CloseSelect
		}

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.

3 participants