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

Modal doesn't open without popper.min.js #24304

Closed
iamrealmarsel opened this issue Oct 9, 2017 · 10 comments
Closed

Modal doesn't open without popper.min.js #24304

iamrealmarsel opened this issue Oct 9, 2017 · 10 comments
Labels

Comments

@iamrealmarsel
Copy link

iamrealmarsel commented Oct 9, 2017

Modal component doesn't work without including popper.min.js before bootstrap.js, console shows the error "Bootstrap dropdown require Popper.js"
Bootstrap v4

@Johann-S
Copy link
Member

Johann-S commented Oct 9, 2017

Indeed Bootstrap depends on Popper.js for our Tooltips/Dropdown and Popovers plugins.

@XhmikosR what do you think about moving this test : https://github.com/twbs/bootstrap/blob/v4-dev/js/src/tooltip.js#L15-L21 in our plugins constructor ?
Because folks who don't need to use those components, should be able to do that without this error

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

@Johann-S: wouldn't it then throw for people who don't want to use those components?

How about we have an extra global var like needsPopper and act accordingly in our constructor?

@Johann-S
Copy link
Member

Johann-S commented Oct 9, 2017

Nope people we'll only see this error if they are trying to use our Dropdowns/Tooltips or Popovers
seems a bit redundant because we can simply check if Popper is available or not

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

@Johann-S: please make a PR and CC me.

@Johann-S
Copy link
Member

Johann-S commented Oct 9, 2017

Finally it seems we have no solution here (see : #24306)
so @m01key you have to add Popper.js because 3 of our plugins use it or you can wait for our next release which will add a new dist file with Popper.js inside

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

Either we do what I suggested and throw in the constructor if a component which requires popper is used, or we leave it as it. Otherwise it will throw an error even though one might not use a popper descendant component.

@Johann-S
Copy link
Member

Johann-S commented Oct 9, 2017

Maybe I don't see how you'll do that but for me it wont change anything because our plugins are wrapped into IIFE ended with : })($, Popper)

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

True that. Not sure if there's another way to do things, but with our current implementation I don't think we can do a lot.

@Johann-S
Copy link
Member

Johann-S commented Oct 9, 2017

true, it's hard to add optional dep 😟

@Johann-S
Copy link
Member

Maybe instead of throwing an error we should just use console.warn ? @XhmikosR

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

No branches or pull requests

3 participants