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

Issue loading popover #174

Closed

Conversation

Andrey-Pavlov
Copy link

@Andrey-Pavlov Andrey-Pavlov commented Sep 15, 2016

#172


This change is Reviewable

@Andrey-Pavlov
Copy link
Author

Andrey-Pavlov commented Sep 15, 2016

Webpack:

{ test: /bootstrap[\/\\]js[\/\\]src[\/\\]/, loader: 'imports?jQuery=jquery' },

@sky-coding
Copy link

Unless I'm mistaken, this attempts to load bootstrap src, which is written in ES6 and would require a transpiler to be configured. I would drop bootstrap-loader before installing Babel.

@Judahmeek
Copy link
Contributor

Is that just because of all the additional complexity, @sky-coding?

@justin808
Copy link
Member

I need some more explanation on this change.

Why would one want to use the src rather than the dist? debugging?

What exactly is the problem being solved?

@sky-coding
Copy link

@justin808 the reasoning is that their dist code no longer supports UMD, so by compiling src their code can be loaded modularly. I think my fix in #172 is the appropriate solution to this problem.

@Judahmeek compiling third-party source code during build is just generally a bad idea, especially when it is in a different language from my source code and would require its own compiler.

@justin808
Copy link
Member

@sky-coding @Andrey-Pavlov @Judahmeek we need some agreement on the preferred solution. I'll release once the 3 of you agree, or if we get more consensus from other community participants.

@Judahmeek
Copy link
Contributor

@justin808, I followed @sky-coding's advice and addressed #172 through #175. I believe that @sky-coding will agree with me that this PR is unnecessary.

@Andrey-Pavlov, would you like to make an argument for this PR or do you agree that we can close this?

@justin808
Copy link
Member

@Judahmeek Let's get a PR for the doc change in that case! Thanks for following up!

@Andrey-Pavlov
Copy link
Author

I agree, we can close it. Thank you, everyone! 👍

@Judahmeek
Copy link
Contributor

@justin808 #175 is the doc change PR and you've already merged it.

@justin808 justin808 closed this Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants