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

Explain the "dispose" method appropriately #30838

Merged
merged 4 commits into from Oct 2, 2020

Conversation

rohit2sharma95
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 commented May 15, 2020

Closes #27957

Updated the document for the alert component.

In my opinion, it should be done for all the components that have dispose method.

@twbs twbs deleted a comment from rohit2sharma95 May 16, 2020
@XhmikosR
Copy link
Member

That's not how you rebase :P

@XhmikosR XhmikosR marked this pull request as draft May 20, 2020 16:54
@rohit2sharma95
Copy link
Collaborator Author

My bad @XhmikosR. I actually merged a pull request to update my branch.

@XhmikosR XhmikosR marked this pull request as ready for review May 20, 2020 17:05
@XhmikosR XhmikosR added the docs label Jun 9, 2020
@mdo mdo changed the base branch from master to main June 16, 2020 19:34
@rohit2sharma95 rohit2sharma95 force-pushed the explain-dispose-method branch 2 times, most recently from bf86432 to 45364c9 Compare June 18, 2020 07:59
@XhmikosR
Copy link
Member

XhmikosR commented Jul 5, 2020

@rohit2sharma95 can you please update all the other instance too?

@rohit2sharma95
Copy link
Collaborator Author

Sure @XhmikosR
Let me do it 😊

@XhmikosR
Copy link
Member

XhmikosR commented Jul 6, 2020

@mdo can you have a quick look regarding capitalization et all?

@XhmikosR XhmikosR added this to Inbox in v5.0.0-alpha2 via automation Jul 6, 2020
@XhmikosR XhmikosR added this to Inbox in v4.5.1 via automation Jul 6, 2020
v5.0.0-alpha2 automation moved this from Inbox to Review Jul 13, 2020
@mdo
Copy link
Member

mdo commented Jul 14, 2020

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).

@mdo
Copy link
Member

mdo commented Jul 20, 2020

@Johann-S Friendly ping—can you list out what is removed for each component? <3

@rohit2sharma95
Copy link
Collaborator Author

rohit2sharma95 commented Jul 20, 2020

@mdo These are my findings after reading the code
@Johann-S Please correct me where I am wrong :)

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.
This data is stored so that the user can get the actual instance of the component by the DOM element and it is being used in the jQueryInterface as well.

So when calling the dispose method for any component's instance:

  • Alert:
    • Removes this instance from the DOM Element
    • Sets the instance's private properties to null
  • Button:
    • Removes this instance from the DOM Element
    • Sets the instance's private properties to null
  • Carousel:
    • Removes event listeners from the DOM Element
    • Removes this instance from the DOM Element
    • Sets the instance's private properties to null
  • Collapse:
    • Removes this instance from the DOM Element
    • Sets the instance's private properties to null
  • Dropdowns:
    • Removes event listeners from the DOM Element
    • Removes this instance from the DOM Element
    • Destroys popper's instance if created for the dropdown
    • Sets the instance's private properties to null
  • Modals:
    • Removes event listeners from the DOM Elements (from window and document as well)
    • Removes this instance from the DOM Element
    • Sets the instance's private properties to null
  • Toast:
    • Clears timeout and removes show class from the DOM Element if exists
    • Removes event listeners from the DOM Element
    • Removes this instance from the DOM Element
    • Sets the instance's private properties to null
  • Tooltips and popovers:
    • Removes event listeners from the DOM Elements
    • Removes this instance from the DOM Element
    • Destroys popper's instance if created for the tooltip or popover
    • Sets the instance's private properties to null
  • Scrollspy:
    • Removes event listeners from the DOM Element
    • Removes this instance from the DOM Element
    • Sets the instance's private properties to null

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.

@Johann-S
Copy link
Member

it looks good to me @rohit2sharma95 👍

@Johann-S Johann-S mentioned this pull request Aug 3, 2020
3 tasks
@mdo mdo removed this from Inbox in v4.5.1 Aug 4, 2020
@mdo mdo added this to In progress in v4.5.3 via automation Aug 4, 2020
@mdo mdo removed this from Review in v5.0.0-alpha2 Sep 10, 2020
@mdo mdo added this to Inbox in v5.0.0-alpha3 via automation Sep 10, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Oct 1, 2020

@Johann-S what's left here?

@Johann-S
Copy link
Member

Johann-S commented Oct 2, 2020

Nothing to me, we're ready to merge 👍

v5.0.0-alpha3 automation moved this from Inbox to Approved Oct 2, 2020
@rohit2sharma95
Copy link
Collaborator Author

rohit2sharma95 commented Oct 2, 2020

These commits are there in PR #31337 though

@XhmikosR
Copy link
Member

XhmikosR commented Oct 2, 2020 via email

@rohit2sharma95
Copy link
Collaborator Author

Sorry, @XhmikosR I mentioned the wrong PR in the comment above. I mean the changes suggested in this PR are kept in the new PR #31337 created by @mdo that is still in progress.

@XhmikosR
Copy link
Member

XhmikosR commented Oct 2, 2020

True, let's close this one then.

@XhmikosR XhmikosR closed this Oct 2, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Oct 2, 2020

Actually wait, I'd rather merge this one and rebase #31337.

@XhmikosR XhmikosR reopened this Oct 2, 2020
v5.0.0-alpha3 automation moved this from Approved to Inbox Oct 2, 2020
@XhmikosR XhmikosR merged commit e4be064 into twbs:main Oct 2, 2020
v5.0.0-alpha3 automation moved this from Inbox to Shipped Oct 2, 2020
@rohit2sharma95 rohit2sharma95 deleted the explain-dispose-method branch October 2, 2020 13:06
@XhmikosR XhmikosR moved this from Inbox to Shipped in v4.5.3 Oct 2, 2020
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4.5.3
Shipped
v5.0.0-alpha3
  
Shipped
Development

Successfully merging this pull request may close these issues.

Replace use of "destroys" jargon with an appropriate explanation.
4 participants