This looks great! I just have a couple small comments :-)
https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js File app/assets/javascripts/routing.js (right):
https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js#newcode5 app/assets/javascripts/routing.js:5: function _trim(s, char, leading, trailing) { This feels like a generic utility method which could be split out into a separate utility method module and hung off Y.Lang so that it's available application wide.
As a generic utility method it would probably be best solved using a regex. Ln 5-18 could be replaced with:
** WARNING - incoming untested code **
Y.Lang.trimString = function(str, start, end) {
var trimRegex = new RegExp("^[" + start + "]+|[" + end + "]+$","g"); return str.replace(trimRegex, "");
}
https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js#newcode90 app/assets/javascripts/routing.js:90: console.log('URL namespace without path'); If you're creating logs for debugging it's best to use Y.log() because it can then be filtered in or out during debugging instead of always logging to the console.
https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js#newcode96 app/assets/javascripts/routing.js:96: console.log('URL has more than one refernce to same namespace'); s/refernce/reference :-) also see above comment about Y.log()
https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/routing.js#newcode113 app/assets/javascripts/routing.js:113: var u = '/'; u is url? Please don't manually minify - use meaningful variable names </pet peeve> :-)
https://codereview.appspot.com/7327045/
« Back to merge proposal
This looks great! I just have a couple small comments :-)
https:/ /codereview. appspot. com/7327045/ diff/16001/ app/assets/ javascripts/ routing. js javascripts/ routing. js (right):
File app/assets/
https:/ /codereview. appspot. com/7327045/ diff/16001/ app/assets/ javascripts/ routing. js#newcode5 javascripts/ routing. js:5: function _trim(s, char, leading,
app/assets/
trailing) {
This feels like a generic utility method which could be split out into a
separate utility method module and hung off Y.Lang so that it's
available application wide.
As a generic utility method it would probably be best solved using a
regex. Ln 5-18 could be replaced with:
** WARNING - incoming untested code **
Y.Lang.trimString = function(str, start, end) {
var trimRegex = new RegExp("^[" + start + "]+|[" + end + "]+$","g"); trimRegex, "");
return str.replace(
}
https:/ /codereview. appspot. com/7327045/ diff/16001/ app/assets/ javascripts/ routing. js#newcode90 javascripts/ routing. js:90: console.log('URL namespace without
app/assets/
path');
If you're creating logs for debugging it's best to use Y.log() because
it can then be filtered in or out during debugging instead of always
logging to the console.
https:/ /codereview. appspot. com/7327045/ diff/16001/ app/assets/ javascripts/ routing. js#newcode96 javascripts/ routing. js:96: console.log('URL has more than one reference :-) also see above comment about Y.log()
app/assets/
refernce to same namespace');
s/refernce/
https:/ /codereview. appspot. com/7327045/ diff/16001/ app/assets/ javascripts/ routing. js#newcode113 javascripts/ routing. js:113: var u = '/';
app/assets/
u is url? Please don't manually minify - use meaningful variable names
</pet peeve> :-)
https:/ /codereview. appspot. com/7327045/