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

Simplify ScrollSpy config #33250

Merged
merged 4 commits into from Apr 6, 2021
Merged

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Mar 3, 2021

I tried to extract a basicJQueryInterface to be used by other components too

@GeoSot GeoSot requested a review from a team as a code owner March 3, 2021 00:34
@GeoSot
Copy link
Member Author

GeoSot commented Mar 3, 2021

@rohit2sharma95 , @XhmikosR
any thoughts on this?

@rohit2sharma95
Copy link
Collaborator

Your idea is to move the jQueryInterface's code to utils? There are few plugins that use their own interface within jQueryInterface 🤔

static jQueryInterface(config) {
return this.each(function () {
Carousel.carouselInterface(this, config)
})
}

There is also a function defineJQueryPlugin in the utils (if it can be extended instead of writing a new function):

const defineJQueryPlugin = (name, plugin) => {
onDOMContentLoaded(() => {

@XhmikosR
Copy link
Member

XhmikosR commented Mar 3, 2021

Yeah, I'm not sure about this either. We shouldn't overdo it with unnecessary abstractions. So what's the plan here exactly?

@GeoSot
Copy link
Member Author

GeoSot commented Mar 3, 2021

I saw that most of them use the same functionality. So I think it could be moved as a separate function outside the component. At least for the 6 of them for the beginning. Small size improvements as keeping one pattern. (now all have different pattern of usage)

@XhmikosR
Copy link
Member

XhmikosR commented Mar 3, 2021

Please finish with your suggestion so that we can see if it's worth doing it :)

@GeoSot
Copy link
Member Author

GeoSot commented Mar 3, 2021

I could start standardizing a bit the configs and the interfaces, and maybe then we can discus the 'one same function'. Does this sound better?

@GeoSot GeoSot force-pushed the simplify-scrollspy-config branch from 0cc0109 to bc6dfdf Compare April 5, 2021 19:43
@GeoSot GeoSot added this to Inbox in v5.0.0 via automation Apr 5, 2021
@GeoSot GeoSot moved this from Inbox to Approved in v5.0.0 Apr 5, 2021
@XhmikosR XhmikosR merged commit 0b34ff2 into twbs:main Apr 6, 2021
v5.0.0 automation moved this from Approved to Done Apr 6, 2021
@GeoSot GeoSot deleted the simplify-scrollspy-config branch April 6, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants