-
Notifications
You must be signed in to change notification settings - Fork 120
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
feat: add lib and es6 bundles, use closure compiler, remove gulp #70
Conversation
…o fred/use-webpack * 'fred/use-webpack' of https://github.com/imgix/drift: Sync with Prettier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small changes, but by and large this looks good to me. I don't have a ton of experience with these particular build/rollup systems, though, so I'd love to get a second opinion in here first if we can.
webpack.config.js
Outdated
const path = require("path"); | ||
const env = require("yargs").argv.env; // use --env with webpack 2 | ||
const pkg = require("./package.json"); | ||
const UglifyJsPlugin = require("uglifyjs-webpack-plugin"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to be used in this file.
bower.json
Outdated
@@ -25,7 +25,6 @@ | |||
"src", | |||
"test", | |||
"CONTRIBUTING.md", | |||
"gulpfile.js", | |||
"index.html", | |||
"karmaConfig.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the files in this list need updating
CONTRIBUTING.md
Outdated
gulp test-local # run the test against local browsers only (Chrome, Safari, Firefox) | ||
gulp test-full # run the test suite against all supported browsers (using SauceLabs) | ||
npm run build # build Drift from source | ||
npm run build:watch # watch for changes and build automatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Handsome tab spacing is broken here
👏This looks awesome! Also, that run-p package seems really nice. Looks as though it would make working with scripts much easier. |
…o fred/use-webpack * 'fred/use-webpack' of https://github.com/imgix/drift: chore(CONTRIBUTING): fix tab alignment
Description
This PR improves the build step of this library. The minified JS build is reduced from
18K -> 11K = 40% smaller
. The css size is unchanged.There have been some changes to the source code to support the closure compiler. There might be a chance that things that worked before may not work now due to the optimisations it performs. I have tried to cover all common usage, but there may be some edge cases which I have not tested. All the tests still pass, and I've ensured that the tests test the compiled code.
Gulp has also been removed in favour of npm scripts.
New Feature
feat(<area>): added new way to do this cool thing #issue-number
Steps to Test
npm install
to update dependenciesnpm run build
and openindex.html
in a browser, and test that Drift still works normallyimport Drift from 'drift-zoom'