Enabled archors for legs

[calidion, 2010-04-14 02:50:34]

Enabled archors for legs

[baiming, 2010-04-15 08:20:39]

Don't have enough time right now.
The review probably will be started tomorrow.

Sorry for the delay.

On Wed, Apr 14, 2010 at 10:50 AM,  <calidion@gmail.com> wrote:
> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>
> Message:
> Enabled archors for legs
>
>
>
> Please review this at http://codereview.appspot.com/886044/show
>
> Affected files:
>  A     gtfsscheduleviewer/files/pin.png
>  M     gtfsscheduleviewer/files/transit_editor.js
>
>
>



-- 
Ming Bai 白明
Software Engineer
Google Inc.
Tel(Office): 86 (10) 6250-3361
Tel(Cell): +86-159-0153-4908
Email: baiming@google.com

[baiming, 2010-04-16 08:52:48]

Only a couple of minor comments; otherwise it LGTM.

btw, can you provide us the demo link? Thanks!

http://codereview.appspot.com/886044/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/886044/diff/1/2#newcode3126
gtfsscheduleviewer/files/transit_editor.js:3126: var archor = null;
Please replace all "archor" with "anchor"

http://codereview.appspot.com/886044/diff/1/2#newcode3127
gtfsscheduleviewer/files/transit_editor.js:3127: var redIcon8 = null;
Why do you move it out of the if clause?
What does redIcon"8" mean?

http://codereview.appspot.com/886044/diff/1/2#newcode3381
gtfsscheduleviewer/files/transit_editor.js:3381: var shortDist = INFINITY;
Better to be minDist.

http://codereview.appspot.com/886044/diff/1/2#newcode3385
gtfsscheduleviewer/files/transit_editor.js:3385: var pos = null;
Is it used or not?

http://codereview.appspot.com/886044/diff/1/2#newcode3400
gtfsscheduleviewer/files/transit_editor.js:3400: GLog.write("unknown error");
Instead of an unknown error, I think it should be an error of "invalid
polyline".

[calidion, 2010-04-16 09:07:08]

It seems due to the power down of the server,
the demo link was invalid today.
and now it is booted again at:

http://211.100.227.25:8765/

2010/4/16 <baiming@google.com>

> Only a couple of minor comments; otherwise it LGTM.
>
> btw, can you provide us the demo link? Thanks!
>
>
> http://codereview.appspot.com/886044/diff/1/2
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
> http://codereview.appspot.com/886044/diff/1/2#newcode3126
> gtfsscheduleviewer/files/transit_editor.js:3126: var archor = null;
> Please replace all "archor" with "anchor"
>
> http://codereview.appspot.com/886044/diff/1/2#newcode3127
> gtfsscheduleviewer/files/transit_editor.js:3127: var redIcon8 = null;
> Why do you move it out of the if clause?
> What does redIcon"8" mean?
>
> http://codereview.appspot.com/886044/diff/1/2#newcode3381
> gtfsscheduleviewer/files/transit_editor.js:3381: var shortDist =
> INFINITY;
> Better to be minDist.
>
> http://codereview.appspot.com/886044/diff/1/2#newcode3385
> gtfsscheduleviewer/files/transit_editor.js:3385: var pos = null;
> Is it used or not?
>
> http://codereview.appspot.com/886044/diff/1/2#newcode3400
> gtfsscheduleviewer/files/transit_editor.js:3400: GLog.write("unknown
> error");
> Instead of an unknown error, I think it should be an error of "invalid
> polyline".
>
>
> http://codereview.appspot.com/886044/show
>

[calidion, 2010-04-16 09:39:08]

updated

http://codereview.appspot.com/886044/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/886044/diff/1/2#newcode3126
gtfsscheduleviewer/files/transit_editor.js:3126: var archor = null;
On 2010/04/16 08:52:48, baiming wrote:
> Please replace all "archor" with "anchor"

Done.

http://codereview.appspot.com/886044/diff/1/2#newcode3127
gtfsscheduleviewer/files/transit_editor.js:3127: var redIcon8 = null;
On 2010/04/16 08:52:48, baiming wrote:
> Why do you move it out of the if clause?
> What does redIcon"8" mean?

It is because of the intention to change the icon when it is in dragging mode.
8 is the size of the icon;

http://codereview.appspot.com/886044/diff/1/2#newcode3381
gtfsscheduleviewer/files/transit_editor.js:3381: var shortDist = INFINITY;
On 2010/04/16 08:52:48, baiming wrote:
> Better to be minDist.

Done.

http://codereview.appspot.com/886044/diff/1/2#newcode3385
gtfsscheduleviewer/files/transit_editor.js:3385: var pos = null;
On 2010/04/16 08:52:48, baiming wrote:
> Is it used or not?

Done.

http://codereview.appspot.com/886044/diff/1/2#newcode3400
gtfsscheduleviewer/files/transit_editor.js:3400: GLog.write("unknown error");
On 2010/04/16 08:52:48, baiming wrote:
> Instead of an unknown error, I think it should be an error of "invalid
> polyline".

Done.

[baiming, 2010-04-16 10:22:59]

LGTM.

http://codereview.appspot.com/886044/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/886044/diff/1/2#newcode3127
gtfsscheduleviewer/files/transit_editor.js:3127: var redIcon8 = null;
On 2010/04/16 09:39:08, calidion wrote:
> On 2010/04/16 08:52:48, baiming wrote:
> > Why do you move it out of the if clause?
> > What does redIcon"8" mean?
> 
> It is because of the intention to change the icon when it is in dragging mode.
> 8 is the size of the icon;

Please rename it to redIcon8px or smallRedIcon.
redIcon8 indicates there are other icons named redIcon1, redIcon2, ...

[calidion, 2010-04-16 12:39:53]

updated to rev 73

2010/4/16 <baiming@google.com>

> LGTM.
>
>
>
> http://codereview.appspot.com/886044/diff/1/2
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
> http://codereview.appspot.com/886044/diff/1/2#newcode3127
> gtfsscheduleviewer/files/transit_editor.js:3127: var redIcon8 = null;
> On 2010/04/16 09:39:08, calidion wrote:
>
>> On 2010/04/16 08:52:48, baiming wrote:
>> > Why do you move it out of the if clause?
>> > What does redIcon"8" mean?
>>
>
>  It is because of the intention to change the icon when it is in
>>
> dragging mode.
>
>> 8 is the size of the icon;
>>
>
> Please rename it to redIcon8px or smallRedIcon.
> redIcon8 indicates there are other icons named redIcon1, redIcon2, ...
>
>
> http://codereview.appspot.com/886044/show
>