-
-
Notifications
You must be signed in to change notification settings - Fork 172
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
Handle esc key in settings #4702
Conversation
Simplified solution instead #4205 which has too many changes that have impact on all dialogs and the behaviour of buttons etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code LGTM, did not test
just noticed that the event listeners are not cleaned up again, useEffect functions should return a callback for cleanup. |
As you see I noticed that too and added an AbortController which will remove the eventListener if window.__settingsOpened is false or settingsMode === 'main' |
ok the point is yours. maybe add a comment about that it removes the event listener, I didn't knew that new web feature and assumed it would just stop the event listener. |
but still what happens when you leave the settings, there is still a leak, better call the abort controller in the cleanup function. But then again I wonder why you don't just use the proven pattern that we use elsewhere for event listeners in use effects: pseudo code
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are currently some issues and questions that need to be resolved first before we merge it see previous comment and also I wonder why there are two use effects and listeners now
Didn't see that, so of course that is simpler and easier. I adapted the listener in useEffect |
partly solves #4128