-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Explain the "dispose" method appropriately #30838
Conversation
a805a3d
to
0e77075
Compare
0e77075
to
365c21c
Compare
365c21c
to
48eeff7
Compare
That's not how you rebase :P |
ecdbe8d
to
634ddc7
Compare
My bad @XhmikosR. I actually merged a pull request to update my branch. |
bf86432
to
45364c9
Compare
45364c9
to
e42c560
Compare
@rohit2sharma95 can you please update all the other instance too? |
Sure @XhmikosR |
@mdo can you have a quick look regarding capitalization et all? |
If you let me know which one does what in a list here, I can happily update. I'm already riffing on this PR to fix the inconsistency around our methods and options documentation (going all in on the tables instead of a mix of tables and text). |
@Johann-S Friendly ping—can you list out what is removed for each component? <3 |
@mdo These are my findings after reading the code When a constructor is called for any component, it stores the data (the actual instance being created from that constructor) on a private variable (not on the DOM element in real). The association between the private variable and DOM element is done by a key and id, those are registered on the DOM Element. So when calling the
Removes this instance from the DOM Element means removing this instance from the private variable where it was stored and removing the key and id from the actual DOM element. |
it looks good to me @rohit2sharma95 👍 |
@Johann-S what's left here? |
Nothing to me, we're ready to merge 👍 |
These commits are there in PR #31337 though |
Not sure what you mean?
…On Fri, Oct 2, 2020, 12:42 Rohit Sharma ***@***.***> wrote:
These commits are there in PR #31780
<#31780> though
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#30838 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNOQAXAU7WLXAX6TIWTSIWOAHANCNFSM4NBYHZGA>
.
|
True, let's close this one then. |
Actually wait, I'd rather merge this one and rebase #31337. |
Closes #27957
Updated the document for the alert component.
In my opinion, it should be done for all the components that have
dispose
method.