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

create a base component #29370

Merged
merged 7 commits into from
Nov 29, 2020
Merged

create a base component #29370

merged 7 commits into from
Nov 29, 2020

Conversation

Johann-S
Copy link
Member

@Johann-S Johann-S commented Sep 4, 2019

Create a base component which will allow us to mutualize logics across our components

TODO:

Preview: https://deploy-preview-29370--twbs-bootstrap.netlify.app/

@XhmikosR
Copy link
Member

XhmikosR commented Sep 4, 2019

This increases the dist files. I'm not sure about any performance impact either.

We should try and get some benchmarks in before going further with this kind of changes IMO...

@Johann-S
Copy link
Member Author

Johann-S commented Sep 4, 2019

This change doesn't impact perfs, it's just a bit of refactoring to keep our codebase clean. Inheritance is something we already use (Popovers inherit from Tooltip)

@Johann-S
Copy link
Member Author

BTW I plan to add a createInstance method in the base component to allow our users to create instances without using new when they don't need to store an instance

@mdo mdo changed the base branch from master to main June 16, 2020 20:15
@mdo
Copy link
Member

mdo commented Sep 18, 2020

Is this worth revisiting or is it too stale/old?

@Johann-S
Copy link
Member Author

We should investigate a bit more to move the _jQueryInterface inside the BaseComponent

@XhmikosR XhmikosR force-pushed the master-jo-base-component branch 4 times, most recently from 61a458d to b556c8c Compare November 16, 2020 15:18
@Johann-S Johann-S marked this pull request as ready for review November 20, 2020 10:18
@Johann-S Johann-S requested a review from a team as a code owner November 20, 2020 10:18
@Johann-S
Copy link
Member Author

@XhmikosR I worked on that and was able to move dispose in the BaseComponent

But currently I don't think we can do more than that, so this PR as ready to be reviewed

@rohit2sharma95
Copy link
Collaborator

IMO, this PR now gives benefit to:

  1. Allow constructors to accept a CSS selector #32245 (Logic for CSS selectors can be written in BaseComponent)
  2. refactor: use a Map instead of an Object in dom/data #32180 (Includes tests for getInstance method)
  3. Ability to add listeners directly to objects like Modals/Collapse/etc #32184 (Event listeners can be written in BaseComponent 🤔)

js/src/base-component.js Outdated Show resolved Hide resolved
@XhmikosR XhmikosR merged commit 04674f8 into main Nov 29, 2020
@XhmikosR XhmikosR deleted the master-jo-base-component branch November 29, 2020 18:58
@XhmikosR
Copy link
Member

XhmikosR commented Dec 2, 2020

@Johann-S @rohit2sharma95 should we build the base plugin too? Currently it's inlined in each plugin's dist file in js/dist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants