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

Refactor components to use a utility function to define jQuery plugins #32285

Merged
merged 8 commits into from Dec 8, 2020

Conversation

alpadev
Copy link
Contributor

@alpadev alpadev commented Nov 30, 2020

Closes #32286

TODO:

  • Tighten Bundlewatch limits before merging

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

@@ -203,5 +219,6 @@ export {
noop,
reflow,
getjQuery,
onDOMContentLoaded
onDOMContentLoaded,
Copy link
Member

Choose a reason for hiding this comment

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

Is the onDOMContentLoaded export still used?

Copy link
Contributor Author

@alpadev alpadev Nov 30, 2020

Choose a reason for hiding this comment

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

Not that I'm aware of. I kept it like that because there is already a spec for it and to keep it testable. Also I wasn't sure if there would be some use case for it in the future.

Copy link
Member

Choose a reason for hiding this comment

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

As the last resort we could rename this to _onDOMContentLoaded, but since we test jQuery already, testing these individually might not be needed after all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems a better approach

Copy link
Member

Choose a reason for hiding this comment

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

Actually, NVM my comment. Util isn't exposed in our public API https://github.com/twbs/bootstrap/blob/main/js/index.umd.js

So, I'd say @alpadev you can go ahead with this PR and ping us when it's ready. 👍

@XhmikosR
Copy link
Member

XhmikosR commented Dec 2, 2020

My only concern is that this shouldn't be part of the public API since it's not used anywhere else. That being said, there must be some way we can still test it.

@rohit2sharma95 thoughts?

@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Dec 2, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Dec 2, 2020

BTW thanks for the PR, it definitely reduces the code and duplication :)

@XhmikosR XhmikosR changed the title Refactor components to use an utility function to define jquery plugins Refactor components to use a utility function to define jquery plugins Dec 4, 2020
@alpadev alpadev marked this pull request as ready for review December 4, 2020 17:13
@alpadev alpadev requested a review from a team as a code owner December 4, 2020 17:13
v5.0.0-beta2 automation moved this from Inbox to Approved Dec 7, 2020
@XhmikosR XhmikosR changed the title Refactor components to use a utility function to define jquery plugins Refactor components to use a utility function to define jQuery plugins Dec 8, 2020
@XhmikosR XhmikosR merged commit 85208ae into twbs:main Dec 8, 2020
v5.0.0-beta2 automation moved this from Approved to Done Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.0.0-beta2
  
Done
Development

Successfully merging this pull request may close these issues.

Refactor components to use an utility function to define jquery plugins
3 participants