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.

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

Add getOrCreateInstance method in base-component #33276

Merged
merged 13 commits into from Jun 3, 2021
Merged

Add getOrCreateInstance method in base-component #33276

merged 13 commits into from Jun 3, 2021

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Mar 5, 2021 •
edited 

   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
Collaborator

@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 😁

js/src/base-component.js Outdated Show resolved Hide resolved
js/src/tooltip.js Outdated Show resolved Hide resolved
@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

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

XhmikosR
XhmikosR reviewed Mar 10, 2021
View reviewed changes
js/src/collapse.js Outdated Show resolved Hide resolved
XhmikosR
XhmikosR reviewed Mar 10, 2021
View reviewed changes
js/src/popover.js Outdated Show resolved Hide resolved
XhmikosR
XhmikosR reviewed Mar 10, 2021
View reviewed changes
js/src/tooltip.js Outdated Show resolved Hide resolved
@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
@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

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
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
js/src/offcanvas.js Outdated Show resolved Hide resolved
v5.0.2 automation moved this from Review to Approved Jun 2, 2021
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.

@alpadev alpadev moved this from Approved to Review in v5.0.2 Jun 2, 2021
v5.0.2 automation moved this from Review to Approved Jun 2, 2021
rohit2sharma95
rohit2sharma95 approved these changes Jun 2, 2021
View reviewed changes
Copy link
Collaborator

@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/base-component.spec.js Show resolved Hide resolved
js/tests/unit/tooltip.spec.js
@@ -3,7 +3,7 @@ import EventHandler from '../../src/dom/event-handler'
import { noop } from '../../src/util/index'

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

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

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 😋

js/src/offcanvas.js Outdated Show resolved Hide resolved
@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
v5.0.2 automation moved this from Approved to Done 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 <rohit2sharma95@gmail.com>
Co-authored-by: XhmikosR <xhmikosr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alpadev alpadev alpadev approved these changes

@rohit2sharma95 rohit2sharma95 rohit2sharma95 approved these changes

@XhmikosR XhmikosR Awaiting requested review from XhmikosR

Assignees
No one assigned
Labels
js refactoring v5
Projects
No open projects
v5.0.2
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@GeoSot @XhmikosR @alecpl @rohit2sharma95 @alpadev