mpodwysocki
commented
8 years ago Review status: 0 of 110 files reviewed at latest revision, 8 unresolved discussions.
_.gitignore, line 49 [r1] (raw file):_ I'd also add:
Node
node_modules npm-debug.log
IDEs
.vscode .idea
app/jxcore/app.js, line 2 [r1] (raw file): What from ESNext are we using here? And if so should it be in our regular .jshintrc file?
app/jxcore/hook.js, line 1 [r1] (raw file): missing 'use strict'
_app/jxcore/public/elements/routing.html, line 16 [r1] (raw file):_ That's a redundant check since myApp.username truthy check already checks for '' for example:
var foo = ''; if (foo) { console.log('hi'); /* never runs */ }
_app/jxcore/routes/_api.js, line 11 [r1] (raw file):_ Should there be null checks as well? You could do the same here with req.body.username == null which checks for null and undefined
_app/jxcore/routes/_api.js, line 19 [r1] (raw file):_ The lengths can't be less than zero
_test/helpers/caps.js, line 1 [r1] (raw file):_ strict mode missing
test/helpers/setup.js, line 1 [r1] (raw file): missing strict
Comments from the review on Reviewable.io
deadlyfingers
Updates:
@yaronyg After merge should we take opportunity to clean branches and then switch to issue branching system?