Issue 190083: change display of the bottom info
Descriptionmainly changed from fetchTripRows to showInfoOfTripRouteAndPattern , a member function of Trip.
no longer sending XMLHTTPRequests, but using fetched data instead.
Patch Set 1 #Patch Set 2 : updated #
Total comments: 19
Patch Set 3 : updated #Patch Set 4 : update #
Total comments: 2
Patch Set 5 : updated #
Total comments: 6
Patch Set 6 : updated #
MessagesTotal messages: 17
LGTM for python part. BTW, please make your changelist description more clear, "change display of the bottom info" from what to what? And provide a link to your demo. Thanks! 2010/1/20 <calidion@gmail.com> > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, > > > > Please review this at http://codereview.appspot.com/190083/show > > Affected files: > M gtfsscheduleviewer/files/index.html > M gtfsscheduleviewer/files/transit_editor.js > D gtfsscheduleviewer/files/trip.js > M schedule_editor.py > > > -- Best Regards, Wei Liu 86-10-62503256(o)
Sign in to reply to this message.
Probably I can't start the review until tonight, please hold on. 2010/1/20 <calidion@gmail.com>: > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, > > > > Please review this at http://codereview.appspot.com/190083/show > > Affected files: > M gtfsscheduleviewer/files/index.html > M gtfsscheduleviewer/files/transit_editor.js > D gtfsscheduleviewer/files/trip.js > M schedule_editor.py > > > -- Ming Bai 白明 Software Engineer Google Inc. Tel(Office): 86 (10) 6250-3361 Tel(Cell): +86-159-0153-4908 Email: baiming@google.com
Sign in to reply to this message.
mainly changed from fetchTripRows to showInfoOfTripRouteAndPattern , a member function of Trip. no longer sending XMLHTTPRequests, but using fetched data instead. server url: http://211.100.227.25:8765/ 2010/1/20 Wei Liu <weiliu@google.com> > LGTM for python part. > > BTW, please make your changelist description more clear, "change display of > the bottom info" from what to what? And provide a link to your demo. > > Thanks! > > 2010/1/20 <calidion@gmail.com> > > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, >> >> >> >> Please review this at http://codereview.appspot.com/190083/show >> >> Affected files: >> M gtfsscheduleviewer/files/index.html >> M gtfsscheduleviewer/files/transit_editor.js >> D gtfsscheduleviewer/files/trip.js >> M schedule_editor.py >> >> >> > > > -- > Best Regards, > Wei Liu > 86-10-62503256(o) >
Sign in to reply to this message.
2010/1/20 李白，字一日 <calidion@gmail.com> > mainly changed from fetchTripRows to showInfoOfTripRouteAndPattern , > a member function of Trip. > > no longer sending XMLHTTPRequests, but using fetched data instead. > Please add the above into your issue description instead of the current "Issue 190083: change display of the bottom info". > > server url: > > http://211.100.227.25:8765/ > Does your change introduce any UI change? If yes, please point out which part and how it looks before and after your change. > > > 2010/1/20 Wei Liu <weiliu@google.com> > > LGTM for python part. >> >> BTW, please make your changelist description more clear, "change display >> of the bottom info" from what to what? And provide a link to your demo. >> >> Thanks! >> >> 2010/1/20 <calidion@gmail.com> >> >> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, >>> >>> >>> >>> Please review this at http://codereview.appspot.com/190083/show >>> >>> Affected files: >>> M gtfsscheduleviewer/files/index.html >>> M gtfsscheduleviewer/files/transit_editor.js >>> D gtfsscheduleviewer/files/trip.js >>> M schedule_editor.py >>> >>> >>> >> >> >> -- >> Best Regards, >> Wei Liu >> 86-10-62503256(o) >> > > -- Best Regards, Wei Liu 86-10-62503256(o)
Sign in to reply to this message.
2010/1/20 Wei Liu <weiliu@google.com> > > > 2010/1/20 李白，字一日 <calidion@gmail.com> > > mainly changed from fetchTripRows to showInfoOfTripRouteAndPattern , >> a member function of Trip. >> >> no longer sending XMLHTTPRequests, but using fetched data instead. >> > > Please add the above into your issue description instead of the current > "Issue 190083: change display of the bottom info". > updated > >> server url: >> >> http://211.100.227.25:8765/ >> > > Does your change introduce any UI change? If yes, please point out which > part and how it looks before and after your change. > no ui change currently. > >> >> 2010/1/20 Wei Liu <weiliu@google.com> >> >> LGTM for python part. >>> >>> BTW, please make your changelist description more clear, "change display >>> of the bottom info" from what to what? And provide a link to your demo. >>> >>> Thanks! >>> >>> 2010/1/20 <calidion@gmail.com> >>> >>> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, >>>> >>>> >>>> >>>> Please review this at http://codereview.appspot.com/190083/show >>>> >>>> Affected files: >>>> M gtfsscheduleviewer/files/index.html >>>> M gtfsscheduleviewer/files/transit_editor.js >>>> D gtfsscheduleviewer/files/trip.js >>>> M schedule_editor.py >>>> >>>> >>>> >>> >>> >>> -- >>> Best Regards, >>> Wei Liu >>> 86-10-62503256(o) >>> >> >> > > > -- > Best Regards, > Wei Liu > 86-10-62503256(o) >
Sign in to reply to this message.
2010/1/20 李白，字一日 <calidion@gmail.com> > >> Does your change introduce any UI change? If yes, please point out which >> part and how it looks before and after your change. >> > no ui change currently. > So what does "change display of the bottom info" mean? I thought it was UI change from its wording. But it now makes me confusing. -- Best Regards, Wei Liu 86-10-62503256(o)
Sign in to reply to this message.
sorry for the misunderstanding. what i mean is the change of the way the bottom info displayed from original fetchTripRows to member function showInfoOfTripRouteAndPattern, and never send new http request to fetch new data, use old data instead. 2010/1/20 Wei Liu <weiliu@google.com> > > > 2010/1/20 李白，字一日 <calidion@gmail.com> > >> >>> Does your change introduce any UI change? If yes, please point out which >>> part and how it looks before and after your change. >>> >> no ui change currently. >> > > So what does "change display of the bottom info" mean? I thought it was UI > change from its wording. But it now makes me confusing. > > -- > Best Regards, > Wei Liu > 86-10-62503256(o) >
Sign in to reply to this message.
http://codereview.appspot.com/190083/diff/1005/1006 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/190083/diff/1005/1006#newcode40 gtfsscheduleviewer/files/transit_editor.js:40: trips = {}; It's fine to use such kind of simple name since they are all in the anonymous function body, but you should forget the "var". http://codereview.appspot.com/190083/diff/1005/1006#newcode812 gtfsscheduleviewer/files/transit_editor.js:812: * Get Html info of Route Ah, my fault. As I am always talking about adding jsDoc, I forgot to mention that we also need a @return annotation in case the function returns a value. Taking this function as example, @return {string} The formatted HTML for showing the basic information of the route. Please go through the code, and add @return's for all the needed functions. http://codereview.appspot.com/190083/diff/1005/1006#newcode814 gtfsscheduleviewer/files/transit_editor.js:814: Route.prototype.getInfoHtml = function(data) { Is parameter "data" used? http://codereview.appspot.com/190083/diff/1005/1006#newcode815 gtfsscheduleviewer/files/transit_editor.js:815: var keys = ['id', 'shortName', 'longName', 'type', 'agencyId', 'desc', 'url', Better to use a map <key -> raw key> to do this, and make it a static member of Route. i,e. /** * Map from field name to raw key name in GTFS. * @type Object.<string> * */ Route.GtfsKeys = { 'id': 'route_id', 'shortName': 'route_short_name', ... }; You don't need to be concerned about the loop order of the object, since most (all) modern browsers preserve the natural order of the keys as defined. http://codereview.appspot.com/190083/diff/1005/1006#newcode957 gtfsscheduleviewer/files/transit_editor.js:957: * Show bottom info for current Trip object Does it mean "show the current trip's information in the bottom area of the page", or "the trip has some kind of bottom information, rather than top, then we show it"? If the former, let's rephrase the comment. It's OK to just use what I wrote there. http://codereview.appspot.com/190083/diff/1005/1006#newcode962 gtfsscheduleviewer/files/transit_editor.js:962: var svgInfo = this.getSVGTag('/ttablegraph?height=100&trip=' + this.id, 'height=\'115\' width=\'100%\''); Line length. Use double quotes for HTML content, that can avoid \'. i.e, '(single quote for the whole string)height="(double quote for the HTML)115" ' It is also a convention. http://codereview.appspot.com/190083/diff/1005/1006#newcode978 gtfsscheduleviewer/files/transit_editor.js:978: * the SVG and attributes is inserted directly into the object or embed Use @param for describing the parameters. For attributes, how about using a <key -> value> map, and leave the string concatenation work to the implementation? Passing {'width': '100%', 'height':'115'} is clearer than the string containing inner and outer quotes. http://codereview.appspot.com/190083/diff/1005/1006#newcode981 gtfsscheduleviewer/files/transit_editor.js:981: Remove this blank line. http://codereview.appspot.com/190083/diff/1005/1006#newcode1008 gtfsscheduleviewer/files/transit_editor.js:1008: var rawKeys = ['trip_id', 'trip_headsign', 'route_id', 'service_id', Use similar approach of the <key -> raw key> map.
Sign in to reply to this message.
On Thu, Jan 21, 2010 at 10:33 AM, <baiming@google.com> wrote: > > http://codereview.appspot.com/190083/diff/1005/1006 > File gtfsscheduleviewer/files/transit_editor.js (right): > > http://codereview.appspot.com/190083/diff/1005/1006#newcode40 > gtfsscheduleviewer/files/transit_editor.js:40: trips = {}; > It's fine to use such kind of simple name since they are all in the > anonymous function body, but you should forget the "var". Sorry, s/should/should not. > > http://codereview.appspot.com/190083/diff/1005/1006#newcode812 > gtfsscheduleviewer/files/transit_editor.js:812: * Get Html info of Route > Ah, my fault. As I am always talking about adding jsDoc, I forgot to > mention that we also need a @return annotation in case the function > returns a value. > > Taking this function as example, > > @return {string} The formatted HTML for showing the basic information of > the route. > > Please go through the code, and add @return's for all the needed > functions. > > http://codereview.appspot.com/190083/diff/1005/1006#newcode814 > gtfsscheduleviewer/files/transit_editor.js:814: > Route.prototype.getInfoHtml = function(data) { > Is parameter "data" used? > > http://codereview.appspot.com/190083/diff/1005/1006#newcode815 > gtfsscheduleviewer/files/transit_editor.js:815: var keys = ['id', > 'shortName', 'longName', 'type', 'agencyId', 'desc', 'url', > Better to use a map <key -> raw key> to do this, and make it a static > member of Route. > > i,e. > /** > * Map from field name to raw key name in GTFS. > * @type Object.<string> > * > */ > Route.GtfsKeys = { > 'id': 'route_id', > 'shortName': 'route_short_name', > ... > }; > > You don't need to be concerned about the loop order of the object, since > most (all) modern browsers preserve the natural order of the keys as > defined. > > http://codereview.appspot.com/190083/diff/1005/1006#newcode957 > gtfsscheduleviewer/files/transit_editor.js:957: * Show bottom info for > current Trip object > Does it mean "show the current trip's information in the bottom area of > the page", or "the trip has some kind of bottom information, rather than > top, then we show it"? > > If the former, let's rephrase the comment. It's OK to just use what I > wrote there. > > http://codereview.appspot.com/190083/diff/1005/1006#newcode962 > gtfsscheduleviewer/files/transit_editor.js:962: var svgInfo = > this.getSVGTag('/ttablegraph?height=100&trip=' + this.id, > 'height=\'115\' width=\'100%\''); > Line length. > Use double quotes for HTML content, that can avoid \'. > > i.e, '(single quote for the whole string)height="(double quote for the > HTML)115" ' > > It is also a convention. > > http://codereview.appspot.com/190083/diff/1005/1006#newcode978 > gtfsscheduleviewer/files/transit_editor.js:978: * the SVG and attributes > is inserted directly into the object or embed > Use @param for describing the parameters. > > For attributes, how about using a <key -> value> map, and leave the > string concatenation work to the implementation? > Passing {'width': '100%', 'height':'115'} is clearer than the string > containing inner and outer quotes. > > http://codereview.appspot.com/190083/diff/1005/1006#newcode981 > gtfsscheduleviewer/files/transit_editor.js:981: > Remove this blank line. > > http://codereview.appspot.com/190083/diff/1005/1006#newcode1008 > gtfsscheduleviewer/files/transit_editor.js:1008: var rawKeys = > ['trip_id', 'trip_headsign', 'route_id', 'service_id', > Use similar approach of the <key -> raw key> map. > > http://codereview.appspot.com/190083/show > -- Ming Bai 白明 Software Engineer Google Inc. Tel(Office): 86 (10) 6250-3361 Tel(Cell): +86-159-0153-4908 Email: baiming@google.com
Sign in to reply to this message.
updated. http://codereview.appspot.com/190083/diff/1005/1006 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/190083/diff/1005/1006#newcode40 gtfsscheduleviewer/files/transit_editor.js:40: trips = {}; On 2010/01/21 02:33:03, baiming wrote: > It's fine to use such kind of simple name since they are all in the anonymous > function body, but you should forget the "var". Done. http://codereview.appspot.com/190083/diff/1005/1006#newcode812 gtfsscheduleviewer/files/transit_editor.js:812: * Get Html info of Route On 2010/01/21 02:33:03, baiming wrote: > Ah, my fault. As I am always talking about adding jsDoc, I forgot to mention > that we also need a @return annotation in case the function returns a value. > > Taking this function as example, > > @return {string} The formatted HTML for showing the basic information of the > route. > > Please go through the code, and add @return's for all the needed functions. Done. http://codereview.appspot.com/190083/diff/1005/1006#newcode814 gtfsscheduleviewer/files/transit_editor.js:814: Route.prototype.getInfoHtml = function(data) { On 2010/01/21 02:33:03, baiming wrote: > Is parameter "data" used? Done. http://codereview.appspot.com/190083/diff/1005/1006#newcode815 gtfsscheduleviewer/files/transit_editor.js:815: var keys = ['id', 'shortName', 'longName', 'type', 'agencyId', 'desc', 'url', On 2010/01/21 02:33:03, baiming wrote: > Better to use a map <key -> raw key> to do this, and make it a static member of > Route. > > i,e. > /** > * Map from field name to raw key name in GTFS. > * @type Object.<string> > * > */ > Route.GtfsKeys = { > 'id': 'route_id', > 'shortName': 'route_short_name', > ... > }; > > You don't need to be concerned about the loop order of the object, since most > (all) modern browsers preserve the natural order of the keys as defined. It seems that it is not necessary to make it a static member so far which is not invoked by any other functions. http://codereview.appspot.com/190083/diff/1005/1006#newcode957 gtfsscheduleviewer/files/transit_editor.js:957: * Show bottom info for current Trip object On 2010/01/21 02:33:03, baiming wrote: > Does it mean "show the current trip's information in the bottom area of the > page", or "the trip has some kind of bottom information, rather than top, then > we show it"? > > If the former, let's rephrase the comment. It's OK to just use what I wrote > there. it is not trip's information, as the name indicates, it shows Trip, Route and Pattern related information to current trip. http://codereview.appspot.com/190083/diff/1005/1006#newcode962 gtfsscheduleviewer/files/transit_editor.js:962: var svgInfo = this.getSVGTag('/ttablegraph?height=100&trip=' + this.id, 'height=\'115\' width=\'100%\''); On 2010/01/21 02:33:03, baiming wrote: > Line length. > Use double quotes for HTML content, that can avoid \'. > > i.e, '(single quote for the whole string)height="(double quote for the HTML)115" > ' > > It is also a convention. Done. http://codereview.appspot.com/190083/diff/1005/1006#newcode978 gtfsscheduleviewer/files/transit_editor.js:978: * the SVG and attributes is inserted directly into the object or embed On 2010/01/21 02:33:03, baiming wrote: > Use @param for describing the parameters. > > For attributes, how about using a <key -> value> map, and leave the string > concatenation work to the implementation? > Passing {'width': '100%', 'height':'115'} is clearer than the string containing > inner and outer quotes. not necessary for the time being. and we will introduce unnecessary conversion from Object to String if we do like this. http://codereview.appspot.com/190083/diff/1005/1006#newcode981 gtfsscheduleviewer/files/transit_editor.js:981: On 2010/01/21 02:33:03, baiming wrote: > Remove this blank line. Done. http://codereview.appspot.com/190083/diff/1005/1006#newcode1008 gtfsscheduleviewer/files/transit_editor.js:1008: var rawKeys = ['trip_id', 'trip_headsign', 'route_id', 'service_id', On 2010/01/21 02:33:03, baiming wrote: > Use similar approach of the <key -> raw key> map. Done.
Sign in to reply to this message.
Almost there. Thanks for the change. http://codereview.appspot.com/190083/diff/1005/1006 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/190083/diff/1005/1006#newcode978 gtfsscheduleviewer/files/transit_editor.js:978: * the SVG and attributes is inserted directly into the object or embed On 2010/01/21 06:06:16, calidion wrote: > On 2010/01/21 02:33:03, baiming wrote: > > Use @param for describing the parameters. > > > > For attributes, how about using a <key -> value> map, and leave the string > > concatenation work to the implementation? > > Passing {'width': '100%', 'height':'115'} is clearer than the string > containing > > inner and outer quotes. > > not necessary for the time being. and we will introduce unnecessary conversion > from Object to String if we do like this. > It at least can reduce the possibility to generate typos. if one day we need to specify more than ten attributes, how will the string look like? Machine always does better than us in the dirty and error-prone work. The conversion from object to string is not a pain actually, in either coding, or efficiency which can be ignored. I don't insist on the change, just want you to keep that in mind. http://codereview.appspot.com/190083/diff/17/18#newcode971 gtfsscheduleviewer/files/transit_editor.js:971: if (map) { The new description looks better :)
Sign in to reply to this message.
updated. http://codereview.appspot.com/190083/diff/17/18 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/190083/diff/17/18#newcode971 gtfsscheduleviewer/files/transit_editor.js:971: * Show related Trip, Route and Pattern information in bottombar On 2010/01/21 06:33:49, baiming wrote: > The new description looks better :) Done.
Sign in to reply to this message.
http://codereview.appspot.com/190083/diff/1017/1018 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/190083/diff/1017/1018#newcode997 gtfsscheduleviewer/files/transit_editor.js:997: * @param {String} attributes Attributes for svg object Change string to Object in @param accordingly. http://codereview.appspot.com/190083/diff/1017/1018#newcode1005 gtfsscheduleviewer/files/transit_editor.js:1005: strAttr += ' '; Why do we need this trailing space? http://codereview.appspot.com/190083/diff/1017/1018#newcode1010 gtfsscheduleviewer/files/transit_editor.js:1010: + src + "' " + strAttr + '></embed>'; I guess here is a bug that the opening quote for src is " but the closing one is '. Change it to + src + '" ' And, use four-char indentation when breaking line. return '<embed... + ... instead of return '<embed... + ...
Sign in to reply to this message.
updated http://codereview.appspot.com/190083/diff/1017/1018 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/190083/diff/1017/1018#newcode997 gtfsscheduleviewer/files/transit_editor.js:997: * @param {String} attributes Attributes for svg object On 2010/01/21 07:04:43, baiming wrote: > Change string to Object in @param accordingly. Done. http://codereview.appspot.com/190083/diff/1017/1018#newcode1005 gtfsscheduleviewer/files/transit_editor.js:1005: strAttr += ' '; On 2010/01/21 07:04:43, baiming wrote: > Why do we need this trailing space? Done. http://codereview.appspot.com/190083/diff/1017/1018#newcode1010 gtfsscheduleviewer/files/transit_editor.js:1010: + src + "' " + strAttr + '></embed>'; On 2010/01/21 07:04:43, baiming wrote: > I guess here is a bug that the opening quote for src is " but the closing one is > '. > Change it to > + src + '" ' > > And, use four-char indentation when breaking line. > > return '<embed... > + ... > > instead of > > return '<embed... > + ... Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
updated to rev 48 2010/1/21 <baiming@google.com> > LGTM > > > http://codereview.appspot.com/190083/show >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
