DDMAL / pixel_wrapper

Rodan Pixel.js Wrapper
Other
0 stars 1 forks source link

Implement unit tests and consistent codestyle, fix syntax issues #32

Closed EricHanLiu closed 6 years ago

EricHanLiu commented 6 years ago

Unit tests using Jest and Selenium, small syntax errors fixed to remove Lint JSHint warnings, codestyle consistency enforced

Ensured compatibility with Travis and fixed build issues

Documented everything in README

vigliensoni commented 6 years ago

@deepio Can you check this pull request?

deepio commented 6 years ago

@EricHanLiu In the beginning, there are a few large commits where you try to fix multiple unrelated things, and then you change a small section later (fixing a fix, of a specific section of another fix). Try to have a cleaner git history because it makes it harder to track what you did during review. Keep your commits small, think about it as if you're limiting a commit to the length of something specific that you may need to 'revert' in the future. And you can also delete a pushed commit, with some restrictions obviously. I'm definitely not asking you to revert all the changes and reorganize them now, that would be a huge waste of time.

now is undefined because its commented out here: https://github.com/DDMAL/pixel_wrapper/blob/e0750621228faab217c838114ba5c73f611dec06/source/js/interpolate-animation.js#L11-L22

Maybe they thought now = () => { return performance.now(); }; looked ugly with two semicolons, but if that's the case I would have picked a different code style or (if eslint+jshint supports it) made some minor adjustments to the rules.

EricHanLiu commented 6 years ago

@deepio Yeah the JSHint warning was because there weren't semicolons, so they commented that section out. In general I didn't change the codestyle of what was already written, just tried to fix the JSHint warnings wherever I could (missing semicolons, missing break statement).

Should I add the semicolons there and remove the // jshint ignore on the now()statements

EricHanLiu commented 6 years ago

Will try to move the tests to target a new server, only hosting once the tests are run (remove dependence on Rodan running)

EricHanLiu commented 6 years ago

Okay all the changes have been made, tests all pass now and no longer require the Rodan server to be running. removed the travis environment variables as well

EricHanLiu commented 6 years ago

can I merge this