Modal: fix dispose function
#38857
Open
Modal: fix dispose function
#38857
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GeoSot
reviewed
Jul 6, 2023
js/src/modal.js
Outdated
| @@ -139,13 +139,17 @@ class Modal extends BaseComponent { | |||
| } | |||
|
|
|||
| dispose() { | |||
| EventHandler.off(window, EVENT_KEY) | |||
| EventHandler.off(this._dialog, EVENT_KEY) | |||
| this._element.addEventListener(EVENT_HIDDEN, () => { | |||
There was a problem hiding this comment.
Suggested change
| this._element.addEventListener(EVENT_HIDDEN, () => { | |
| EventHandler.on(this._element, EVENT_HIDDEN, () => { |
try this syntax to be aligned with the rest codebase ;wink
There was a problem hiding this comment.
It would be great to add a test too, supporting your solution
NOTE for reviewers:
The proposed change is valid.
My only consideration is, whether we should handle such situations or not. In case we decide to handle them, we should also think about the other components too
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Call a function that removes all body attributes before disposing the modal itself.
However, I'm not quite sure about my comprehension of our doc explanation about the dispose function. Shall it remove the modal element from the DOM or shall it just remove the modal object instance ? both ?
Motivation & Context
Fix the dispose call when a modal is shown.
Type of changes
Checklist
npm run lint)Live previews
Related issues
Closes #35934.