Skip to content

Add getOrCreateInstance method in base-component #33276

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.

Sign up for GitHub

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

Jump to bottom
Merged
XhmikosR merged 13 commits into from
Jun 3, 2021
Merged

Add getOrCreateInstance method in base-component #33276

XhmikosR merged 13 commits into from
Jun 3, 2021

Conversation

@GeoSot
Copy link
Member

@GeoSot GeoSot commented Mar 5, 2021 •
edited
Loading 

   let data = Data.get(element, DATA_KEY)
  

      if (!data) {
        data = new Alert(element)
      }

can be

  const data = Alert.getOrCreateInstance(element)

@GeoSot GeoSot requested a review from a team as a code owner March 5, 2021 22:53
@GeoSot GeoSot changed the title initial Add 'getInstanceOrNew' method in base-component Mar 5, 2021
rohit2sharma95
rohit2sharma95 reviewed Mar 7, 2021
View reviewed changes
Copy link
Contributor

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

There is commented code also 😁

@GeoSot
Copy link
Member Author

GeoSot commented Mar 7, 2021

Commented code is gone, unrelated reverted, method is renamed :)

@XhmikosR
Copy link
Member

XhmikosR commented Mar 8, 2021

Would this conflict with #33249 and #33250? If so, unsure which should land first.

BTW we don't really test jQuery, just if it works, so I'm a little hesitant :/

@GeoSot
Copy link
Member Author

GeoSot commented Mar 8, 2021 •
edited
Loading

The other two #33249 and #33250, are small easy refactorings , so you can land them first.

As for the jQuery. Every one of them, does not import something new. Only it streamlines the same code snippet

getInstance or make new if config is obj

It is based on the existing tests for Jquery

@GeoSot GeoSot force-pushed the get-instance-or-new branch from 3e188d8 to 6756bd5 Compare March 9, 2021 18:00
XhmikosR
XhmikosR reviewed Mar 10, 2021
View reviewed changes
@GeoSot GeoSot force-pushed the get-instance-or-new branch from 6756bd5 to c53ac10 Compare March 10, 2021 08:31
XhmikosR
XhmikosR reviewed Mar 10, 2021
View reviewed changes
XhmikosR
XhmikosR reviewed Mar 10, 2021
View reviewed changes
@GeoSot GeoSot force-pushed the get-instance-or-new branch 2 times, most recently from 6d6c8fe to e64567f Compare March 16, 2021 21:54
@GeoSot GeoSot marked this pull request as draft March 17, 2021 23:17
@GeoSot GeoSot force-pushed the get-instance-or-new branch 2 times, most recently from c9a751c to a1aeb58 Compare March 23, 2021 23:27
@GeoSot GeoSot force-pushed the get-instance-or-new branch from a1aeb58 to d4ed6fa Compare April 6, 2021 22:26
@GeoSot GeoSot force-pushed the get-instance-or-new branch from d4ed6fa to 2f5e05a Compare April 14, 2021 23:21
@alecpl
Copy link
Contributor

alecpl commented Apr 16, 2021

It would be great if all these @GeoSot's refactoring PRs could make it to 5.0.0. @mdo any chance for that?

@GeoSot is this still a draft? looks like some tests failed.

@XhmikosR
Copy link
Member

XhmikosR commented Apr 16, 2021

I'd rather not get the further refactoring PRs in v5.0.0 to not risk to break something so close to the release. There's plenty of time in 5.0.1 or 5.1.0.

@GeoSot
Copy link
Member Author

GeoSot commented Apr 16, 2021

stable 5.0.0 is the most important thing for now. The next thing, I bet are some of the refactorings

@GeoSot GeoSot force-pushed the get-instance-or-new branch 2 times, most recently from 60f8aff to 6ecb5fd Compare May 11, 2021 08:32
@GeoSot GeoSot force-pushed the get-instance-or-new branch 2 times, most recently from 0be5583 to ce08bb5 Compare May 18, 2021 23:39
@GeoSot GeoSot force-pushed the get-instance-or-new branch 2 times, most recently from c11b060 to 8ca1f63 Compare May 20, 2021 13:38
@GeoSot GeoSot force-pushed the get-instance-or-new branch from 606a1f8 to 2492904 Compare May 22, 2021 21:57
@GeoSot GeoSot requested a review from rohit2sharma95 May 24, 2021 08:03
@GeoSot GeoSot force-pushed the get-instance-or-new branch from ca990fb to 89d1636 Compare May 31, 2021 12:19
@GeoSot GeoSot requested a review from XhmikosR May 31, 2021 12:20
@GeoSot
Copy link
Member Author

GeoSot commented May 31, 2021

I have added the proper documentation. So do we need something more in order to proceed?

alpadev
alpadev reviewed Jun 2, 2021
View reviewed changes
alpadev
alpadev reviewed Jun 2, 2021
View reviewed changes
alpadev
alpadev reviewed Jun 2, 2021
View reviewed changes
alpadev
alpadev reviewed Jun 2, 2021
View reviewed changes
@GeoSot GeoSot force-pushed the get-instance-or-new branch from 89d1636 to 4e7acbe Compare June 2, 2021 09:06
alpadev
alpadev approved these changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@alpadev alpadev left a comment

Choose a reason for hiding this comment

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

LGTM, from my side.

rohit2sharma95
rohit2sharma95 approved these changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

/CC @mdo for the documentation, if required

js/tests/unit/tooltip.spec.js

/** Test helpers */
import { getFixture, clearFixture, jQueryMock, createEvent } from '../helpers/fixture'
import { clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture'
Copy link
Contributor

@rohit2sharma95 rohit2sharma95 Jun 2, 2021

Choose a reason for hiding this comment

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

You must be using any code formatter in your IDE 😅

Copy link
Member Author

@GeoSot GeoSot Jun 2, 2021

Choose a reason for hiding this comment

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

HAHAHA jetBrains like them sorted 😋

@XhmikosR XhmikosR changed the title Add 'getOrCreateInstance' method in base-component Add getOrCreateInstance method in base-component Jun 3, 2021
@XhmikosR XhmikosR merged commit c98657b into twbs:main Jun 3, 2021
@GeoSot GeoSot deleted the get-instance-or-new branch June 3, 2021 22:32
@alpadev alpadev mentioned this pull request Jun 15, 2021
Closed
marvin-hinkley-vortx pushed a commit to Vortx-Inc/bootstrap that referenced this pull request Aug 18, 2021
@GeoSot @rohit2sharma95
@XhmikosR @marvin-hinkley-vortx
Add getOrCreateInstance method in base-component (twbs#33276)
96ea07b 
Co-authored-by: Rohit Sharma <[email protected]>
Co-authored-by: XhmikosR <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XhmikosR XhmikosR Awaiting requested review from XhmikosR

2 more reviewers

@alpadev alpadev alpadev approved these changes

@rohit2sharma95 rohit2sharma95 rohit2sharma95 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

js refactoring v5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@GeoSot @XhmikosR @alecpl @rohit2sharma95 @alpadev