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

manage bootstrap using npm #23

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

wenzowski
Copy link

This effectively reverts the approach of af8c522
keeping the spirit I see behind d2823ab
that is, managing twbs using npm will be
easier to maintain.

Redistributing the minified file prevents
another repo-moved fiasco like #16, while listing
bootstrap as a devDependency means that developers
will be able to peruse the same version of the
unminifed source without it being checked in to
this repository.

This effectively reverts the approach of af8c522
keeping the spirit I see behind d2823ab
that is, managing twbs using npm will be
easier to maintain.

Redistributing the minified file prevents
another repo-moved fiasco like derbyjs#16, while listing
bootstrap as a devDependency means that developers
will be able to peruse the same version of the
unminifed source without it being checked in to
this repository.
@wenzowski
Copy link
Author

This is a much smaller change than it appears due to 3 of the files in css being deleted and the bootstrap.min.css file being moved to the location npm installed it at.

@nateps
Copy link
Contributor

nateps commented Mar 3, 2014

Good idea using npm to manage installing.

I'd include all of the css and less files so that people can require them from within d-bootstrap. It should actually be optional whether or not you want to include the css styles, since you might use a custom compiled version of bootstrap from http://getbootstrap.com/customize/ or you might prefer to require the less files for more flexibility. Perhaps the default should change to not include any styles, and there should be options to include the main styles and the theme styles.

@wenzowski
Copy link
Author

I'm under the impression that bootstrap is not distributed as an npm package, but is only distributed as a bower package, despite the package.json in the repo. I chose to fetch it directly with git rather than add an additional dependency on bower. Now that I think about this, npm should be using github's release url for the tarball. The tarball is currently 5.6MB.

Since it looks like the less compiler will be packaged with Derby for now, perhaps it would be better for loadStyles to just be passed the less. In combination with a config object for modifying paths, this would be the most flexible.

@wenzowski
Copy link
Author

I'd prefer to include styles by default, but agree it is important for customization to be possible.

@wenzowski
Copy link
Author

Using the less directly seems most flexible from a customization perspective, but I'll wait on making that change until derbyjs/derby#388 is reviewed.

@wenzowski wenzowski mentioned this pull request Mar 4, 2014
@wenzowski
Copy link
Author

hmm, while grabbing this over git worked fine the npm docs appear to indicate that anything in node_modules won't be included in a release despite the .gitignore. Perhaps bower makes more sense after all...

@wenzowski
Copy link
Author

finished reading npm and found bundledDependencies which solves the problem

@re1ro
Copy link
Member

re1ro commented Nov 23, 2015

@wenzowski @nateps
I think it was implemented in v0.1.3 and can be closed

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.

3 participants