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

Custom Bundle Script #80

Merged
merged 8 commits into from
Sep 26, 2016
Merged

Custom Bundle Script #80

merged 8 commits into from
Sep 26, 2016

Conversation

RyanZim
Copy link
Contributor

@RyanZim RyanZim commented Sep 26, 2016

Closes #71.

@thednp I also added a watch script, npm run watch. This will watch for changes to the files in lib/ and rerun npm run build for each change (Very handy for development).

I also changed the script tag in index.html to reference the minified bundle (dist/bootstrap-native.min.js) instead of dist/bootstrap-native.js.

Let me know if anything else needs changed.

@RyanZim
Copy link
Contributor Author

RyanZim commented Sep 26, 2016

Thanks!

@thednp
Copy link
Owner

thednp commented Oct 26, 2016

Hey @RyanZim, I'm a little stubborn about performance with these IIFE and the new code for Tooltip and Popover. IIFE always sets a new scope generally and as a result, performance is most likely traded.

I have an idea: I can set a custom variable for each script DATA API to hold the count of each instance, EG:

for ( var m = 0, mdl = Modals.length; m<mdl; m++ ) {
  var modal = Modals[m], options = {};
  // do the dew
}

We should have a unique identifier and as a result we won't mess with the node stuff, so what do you think?

thednp added a commit that referenced this pull request Oct 26, 2016
* Carousel, Popover, Tooltip and Modal no longer store the timer in their prototype, but as a private variable
* Tooltip and Popover improved, now the open method works better allowing their corresponding tooltip to also FADE IN
* Tooltip and Popover are lighter as they export the getScroll() to utils.js file
* Affix and ScrollSpy also uses the exported getScroll()
* A small workaround to eliminate IIFE #80 (comment)
* Documentation updates
thednp added a commit that referenced this pull request Oct 26, 2016
* Carousel, Popover, Tooltip and Modal no longer store the timer in their prototype, but as a private variable
* Tooltip and Popover improved, now the open method works better allowing their corresponding tooltip to also FADE IN
* Tooltip and Popover are lighter as they export the getScroll() to utils.js file
* Affix and ScrollSpy also uses the exported getScroll()
* A small workaround to eliminate IIFE #80 (comment)
* Documentation updates
@thednp
Copy link
Owner

thednp commented Oct 26, 2016

UPDATE: @RyanZim please update from master and make a test. Tell me what you think.

@RyanZim
Copy link
Contributor Author

RyanZim commented Oct 26, 2016

@thednp Looking over your commit, I think the code will work.

My only concern is that you (or another contributer) will someday not think about the identifiers that are declared outside the file and will cause a collision. If you're willing to take that risk (and are sure it won't happen), go for it! It's just a risk I personally wouldn't take.

@thednp
Copy link
Owner

thednp commented Oct 26, 2016

Thanks @RyanZim, I will go ahead and write a reminder into build.js and utils.js about this.

@RyanZim
Copy link
Contributor Author

RyanZim commented Oct 26, 2016

@thednp While you're at it, you might want to make a CONTRIBUTING.md that includes this info.

@thednp
Copy link
Owner

thednp commented Oct 26, 2016

That's a cool idea. I will leave a special section for you regarding NPM/node.js.

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.

2 participants