search

thaliproject / postcardapp

A sample app to demonstrate how to build Thali applications
MIT License
22 stars 5 forks source link

Story 0 - Polymer WebApp #30

Closed deadlyfingers closed 9 years ago

deadlyfingers commented 9 years ago

One additional setup command:

Notes: Cordova config.xml should install 'cordova-plugin-statusbar' plugin. Uses iOS Thali plugin branch.

www/jxcore/cardroutes.js, line 23 [r1] (raw file): Agreed. I reused the login script and quickly modified it to return JSON.

Will revise and refine.

www/jxcore/public/elements/behaviors/path-components.html, line 33 [r1] (raw file): It also tests against floats so a float is not recognised as a valid int.

Tests: http://jsfiddle.net/opfyrqwp/28/

Comments from the review on Reviewable.io

  

Review on Reviewable

yaronyg commented 9 years ago

Reviewed 231 of 244 files at r1. Review status: 231 of 244 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

www/jxcore/cardroutes.js, line 16 [r1] (raw file): Please remove the comments such as this one and the ones below

www/jxcore/cardroutes.js, line 23 [r1] (raw file): It's generally considered to be bad style to nest ifs like this. Typically you would have a if test test for userName.length <= 0 and then immediately do the res.send with user name is required followed by a return exiting the function. Then you would have the db.get and inside of it the first if (on err & err.status) would also end with a return. This would let you out dent a bunch of stuff. Same thing all the way down. E.g. the if (err) body would end in a return as well.

www/jxcore/public/elements/behaviors/multi-platform.html, line 34 [r1] (raw file): Do we really want to log this every time this function is called? In general we should review the console statements and make sure they are actually useful. We don't want to fill up the log with a bunch of not terribly useful stuff and then hide the useful stuff. You should take a look at Winston's logging levels. It would let you log things like this but have them set to 'informational' and be ignored by default.

www/jxcore/public/elements/behaviors/path-components.html, line 33 [r1] (raw file): Not to be too simplistic but um... I would expect a function called _isInt to use parseInt(value, 10) not parseFloat. Is there browser magic here I'm not aware of?

www/jxcore/public/elements/behaviors/tree-traversing.html, line 20 [r1] (raw file): Instead of an if else if tree wouldn't switch do a better job?

Comments from the review on Reviewable.io

deadlyfingers commented 9 years ago

Review status: 231 of 244 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

www/jxcore/cardroutes.js, line 23 [r1] (raw file): Agreed. I reused the login script and quickly modified it to return JSON. Will revise and refine.

www/jxcore/public/elements/behaviors/path-components.html, line 33 [r1] (raw file): It also tests against floats so a float is not recognised as a valid int. Tests: http://jsfiddle.net/opfyrqwp/28/

Comments from the review on Reviewable.io

yaronyg commented 9 years ago

Reviewed 4 of 244 files at r1, 5 of 13 files at r2, 3 of 5 files at r3. Review status: 241 of 246 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

www/jxcore/cardroutes.js, line 26 [r2] (raw file): Isn't this return redundant? You are already inside of a function call and the return just returns from that function call, not from the parent function. Same with the then(function() {}) above.

www/jxcore/cardroutes.js, line 27 [r2] (raw file): Don't you need a return after the db.put? Otherwise the rest of the code will run and i don't think that is what you intended.

Also I don't think you need "else if(err)". I suspect just "if (err)" would be fine.

www/jxcore/cardroutes.js, line 28 [r1] (raw file): The code flow is now fundamentally different than what it was before. Before the code in if (doc.user !== username) only ran if the response from the get on 'me' wasn't an error. Now that code will run no matter what. Is that really what you intended?

www/jxcore/cardroutes.js, line 38 [r2] (raw file): Same comment as above, that returns inside of of a function() aren't going to do anything so they can be removed.

www/jxcore/cardroutes.js, line 95 [r3] (raw file): Why do you divide by 1000? Why not just keep the full name resolution? Ordering will still work and you still have a full resolution date?

www/jxcore/public/elements/page-home/page-home.html, line 162 [r3] (raw file): This is just my ignorance and I apologize but in _saveCard you specify that _reloadData should only be called if a new item is created. But this function is, I thought, used to update an item, not create it. In which case shouldn't the last argument in the __saveCard function call be false?

www/jxcore/public/elements/page-home/page-home.html, line 179 [r3] (raw file): I'm a little confused about _isSaving. The only time it gets flipped from true back to false is if the AJAX request is a complete success. But if there is an error, either in the error function this comment is on or below in the 'else' which issues the alert then isSaving will be true forever. That doesn't sound right.

www/jxcore/public/elements/page-home/page-home.html, line 202 [r3] (raw file): If ironAjax isn't defined then what is supposed to happen? Isn't that bad? But there is no error handling.

www/jxcore/public/scripts/app.js, line 74 [r3] (raw file): I'm guessing that this code comes from http://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript (see the second answer) but unfortunately the legal status of code samples posted to StackOverflow is questionable at best. Per http://meta.stackexchange.com/questions/12527/do-i-have-to-worry-about-copyright-issues-for-code-posted-on-stack-overflow samples at stack overflow are licensed under http://creativecommons.org/licenses/by-sa/3.0/ which only provides license if you are also licensed under cc-by-sa which we sure as heck are not. So we can't safely use this code. In fact, we can't safely use ANY code copied and pasted from stack overflow. Now if you have another source for this exact code which has a clear license then we are good. But you should put a reference to that source in our code so we can make sure we are properly honoring the license. Otherwise the person who wrote the code you used actually has a NPM module (https://github.com/broofa/node-uuid) that does have a license (MIT! Which is the same as our license) and we can use that code without problem. But you will need to use browserify.

Comments from the review on Reviewable.io

deadlyfingers commented 9 years ago

Review status: 241 of 246 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

www/jxcore/cardroutes.js, line 28 [r1] (raw file): It should allow the user to change/update their username, but agree it should not run if there is an error. I will update this block to use 'then & catch' blocks instead which will simplify the logic as I did find it confusing initially. However this feature isn't yet provided to the user as the login screen will only display if there is no 'me' record so it's not essential unless we intend to add an edit username page...

www/jxcore/cardroutes.js, line 95 [r3] (raw file): I was only displaying dates in seconds or longer so was trying to be thrifty with the JSON packet size. But I suppose the extra resolution will help with sub-second ordering.

Comments from the review on Reviewable.io

deadlyfingers commented 9 years ago

Review status: 236 of 246 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

www/jxcore/public/elements/page-home/page-home.html, line 162 [r3] (raw file): It was used for both creating new and updating postcards. But this reloadData param should actually be removed now that refresh button has been removed!

www/jxcore/public/elements/page-home/page-home.html, line 179 [r3] (raw file): _isSaving should be gone now that we are listening for changes instead which is much nicer.

www/jxcore/public/elements/page-home/page-home.html, line 202 [r3] (raw file): It's just checking that the iron-list element exists and has been found within the 'dom-module'. But yes there should be an error raised. I suppose it would be best putting it all on a single line and dropping the conditional as you would automatically get an error saying function 'generateRequest' does not exist.

www/jxcore/public/scripts/app.js, line 74 [r3] (raw file): Need try to verify the origin of this method, but thought it was in the original postcard app.

Comments from the review on Reviewable.io

deadlyfingers commented 9 years ago

@yaronyg that should be all the updates in for r3.

yaronyg commented 9 years ago

Reviewed 3 of 13 files at r2, 2 of 5 files at r3, 10 of 10 files at r4. Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

www/jxcore/app.js, line 71 [r4] (raw file): I will add this as a capability to the REST API. So treat this code as a temporary hack until we come up with the 'official' answer (which may be the same thing =) although I bet it will handle the error event and require some kind of registration.)

www/jxcore/package.json, line 22 [r4] (raw file): Is this really a devDependency? Wouldn't this be needed at runtime in order to make socket.io available in production?

Comments from the review on Reviewable.io

yaronyg commented 9 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

www/jxcore/package.json, line 22 [r4] (raw file): Once you fix this you should submit this PR

Comments from the review on Reviewable.io