Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(16379)

Issue 12874: New webdemo

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years, 2 months ago by Aidamina
Modified:
8 years, 4 months ago
Reviewers:
Aidamina
Base URL:
http://google-sgnodemapper.googlecode.com/svn/trunk/trunk/
Visibility:
Public.

Description

Created a new webdemo which works exactly the same as the old, except that it uses Google's jsapi with jquery to take away all the XmlHttpRequest checks, making it more compatible with other browsers and putting the focus of the javascript on the library rather then the request and parsing of the files. Also revised the parsing a bit to make it possible to add additional regex keys to parse the filelist for site files. So short: Better compatibility More extensible parsing Less code Same functionality

Patch Set 1 #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+146 lines, -0 lines) Patch
demo_new.html View 1 chunk +34 lines, -0 lines 0 comments Download
demo_new.js View 1 chunk +112 lines, -0 lines 7 comments Download

Messages

Total messages: 2
Aidamina
15 years, 2 months ago (2009-02-02 20:54:10 UTC) #1
brad_danga_com
15 years, 2 months ago (2009-02-02 23:27:41 UTC) #2
Code looks good.

Just some random style comments.  (Consistent code style is a very religiously
obeyed rule at Google... Not that any style is better than anything else.  Just
that a project respect any one style consistently so everybody's code looks the
same and it's not distracting to jump between different code.)

http://codereview.appspot.com/12874/diff/1/3
File demo_new.js (right):

http://codereview.appspot.com/12874/diff/1/3#newcode5
Line 5: {
Move { to same line as above.

http://codereview.appspot.com/12874/diff/1/3#newcode6
Line 6: setStatus("Loading nodemapper-base");
4 space indent, not 8.

(Google style is actually 2 space indents & 4 space indents for continued
lines.)

And make sure you always use spaces (never tabs).

http://codereview.appspot.com/12874/diff/1/3#newcode7
Line 7: $.getScript("../nodemapper-base.js",function(){
Style:  space after comma.
Style:  space after ()

http://codereview.appspot.com/12874/diff/1/3#newcode12
Line 12: setStatus("Loading rules for "+sites_to_load+" sites");
Spaces around +

http://codereview.appspot.com/12874/diff/1/3#newcode62
Line 62: // whose HTML for a directory listing isn't even HTML,
inconsistent indentation

http://codereview.appspot.com/12874/diff/1/3#newcode88
Line 88: $("#tosgn_output").html("Converted to: <b>" + output + "</b> (from " +
input + ")");
Try to stick to 80 characters.  Wrap + indent if you need to.

http://codereview.appspot.com/12874/diff/1/3#newcode96
Line 96: var types = ["profile", "content", "atom", "rss", "blog", "openid",
"foaf", "addfriend"];
80 chars.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b