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

Use Webpack & modernize Dropkick #346

Merged
merged 21 commits into from
Jul 30, 2017
Merged

Use Webpack & modernize Dropkick #346

merged 21 commits into from
Jul 30, 2017

Conversation

Robdel12
Copy link
Owner

@Robdel12 Robdel12 commented Jul 24, 2017

What is this?

I hated the gulp setup I made 3 years ago so I decided to use webpack and modernize this repo like it badly needs.

What changed?

  • Use webpack to compile ES6 files
  • Changed dropkick to an ES6 class
  • Rewrote tests to use karma & mocha
  • Cross browser testing with browser stack
  • /lib changed to /src
  • /build changed to /dist
  • Use circleci 2.0 for testing
  • Use yarn for dependencies
  • Use at least node 4
  • Improved test coverage
  • Changed the docs generation to documentation.js

TODOs

  • Finish porting tests
  • Update documentation
  • Update README & Contributing
  • Switch to circle for testing
  • Fill out PR description with all the different changes

import Dropkick from '../src/dropkick.js';


function buildSelect(id, options) {
Copy link
Owner Author

@Robdel12 Robdel12 Jul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Move this to test utils

Copy link
Owner Author

@Robdel12 Robdel12 Jul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Also do an after each clean up of the selects that were built

// assert.equal(navigator.userAgent, "Android");

// dk.refresh();
// });
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably go away and use browser stack to run our tests on mobile devices

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add this comment to the file so it doesn't get lost. 😉

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reached out to browser stack to see if we can get an account to run tests. I’ve got the setup going locally but I’m about to run out of test free test runs

// var dk = new Dropkick("#normal_select");

// dk.open();
// assert.equal(dk.isOpen, true);
// $(".body").trigger("click");
// assert.equal(dk.isOpen, false);
});
// });
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably can get moved into the main test file and create a helper that builds / cleans up iframe creation

<script src="../../lib/polyfills/indexof.js"></script>
<script src="../../lib/dropkick.js"></script> -->
<script src="../../build/js/dropkick.min.js"></script>
<script src="../../dist/dropkick.js"></script>
<script src="jquery-1.11.2.js"></script>
<script src="../../node_modules/qunitjs/qunit/qunit.js"></script>
<script src="../unit/tests.js"></script>
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this

@Robdel12 Robdel12 force-pushed the webpack branch 2 times, most recently from 2e303c1 to c3b4ee8 Compare July 24, 2017 20:54
@Robdel12 Robdel12 changed the title WIP: Use Webpack & modernize Dropkick Use Webpack & modernize Dropkick Jul 25, 2017
@Robdel12 Robdel12 requested a review from wwilsman July 25, 2017 03:09
dist/index.html Outdated
<script>
var select = document.getElementById('normal_select');
var dropkick = new Dropkick(select);
console.log(dropkick);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

dist/index.html Outdated
var select = document.getElementById('normal_select');
var dropkick = new Dropkick(select);
console.log(dropkick);
function close() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will dropkick.close() not work in the onclick up above?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, this was me hacking around before the test suite was up

karma.conf.js Outdated
config.singleRun = true;
config.browsers = ['Chrome_travis_ci'];
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner Author

@Robdel12 Robdel12 Jul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • TODO

// assert.equal(navigator.userAgent, "Android");

// dk.refresh();
// });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add this comment to the file so it doesn't get lost. 😉


// I hate this, but I guess you can't pass disable to uglify..
const plugins = [ extractSass ];
isProduction ? plugins.push(new UglifyJSPlugin()) : null ;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't like this, what about this?

I thought it was tidy and reusable. Food for thought ¯\_(ツ)_/¯

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It felt more dirty than being able to do:

new UglifyJsPlugin({
  disable: !isProduction
})

But I guess this isn’t terrible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I realize the ternary is moot. Just do

isProduction && plugins.push(...);

Or explicitly

if (isProduction) { plugins.push(...); }

karma.conf.js Outdated
// CI config
if (process.env.TRAVIS || process.env.CI) {
config.singleRun = true;
config.browsers = ['Chrome_travis_ci'];
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should include the browser stack launchers too

@Robdel12
Copy link
Owner Author

@wwilsman wanna give one last go?


// I hate this, but I guess you can't pass disable to uglify..
const plugins = [ extractSass ];
isProduction ? plugins.push(new UglifyJSPlugin()) : null ;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I realize the ternary is moot. Just do

isProduction && plugins.push(...);

Or explicitly

if (isProduction) { plugins.push(...); }

Also removed unnessary comment at the top of dropkick
@wwilsman
Copy link
Collaborator

🎉

@Robdel12 Robdel12 merged commit 6700d37 into master Jul 30, 2017
@Robdel12 Robdel12 deleted the webpack branch July 30, 2017 18:01
This was referenced Oct 13, 2017
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