Issue 6625055: Ran GAdash-1.0.js on lint : No errors
|
Created:
11 years, 6 months ago by osama Modified:
11 years, 5 months ago CC:
team-5-guys-project_googlegroups.com Base URL:
http://analytics-api-samples.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionRan GAdash-1.0.js on lint : No errors
Patch Set 1 #
Total comments: 62
Patch Set 2 : Changes to GA DASH API has been done #
Total comments: 10
Patch Set 3 : Corrections were made based on Jeetendra's comments; dimensions in pie chart can now be set by user. #
Total comments: 4
Patch Set 4 : added cahrtOptions object for charts (all should be different according to visualization convention… #Patch Set 5 : abstracted chart options #Patch Set 6 : added chart options for each specific chart/last-n-months/weeks added as well #MessagesTotal messages: 16
I added a couple of style and 1 code related comment. please fix and upload your new file w/fixes to continue the review. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:42: add a space between * and Namespace. Remove extra line. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:48: * Boolean that checks to see if gapi client is loaded. space before *. Keep all * aligned, throughout this doc. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:251: keep this comment https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:340: *with the GA response data. space between * & with https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:580: *http://perfectionkills.com/instanceof-considered-harmful-or-how-to-write-a-robust-isarray/ space after * https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:611: * Pie Chart remove this entire comment https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:618: * Laurent Jacquot, laurent1jacquot@gmail.com move this to top of file https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:621: * @fileoverview remove file overview. Just use a normal comment vs JSdoc...ie /* */ https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:622: * These added functions are designed to create an easier way to build chart remove added https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:623: * Gby wrapping the Chart object. As a result, the code is easier to develop Gby -> by https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:670: if (!this.config.query['end-date'] && this is being replicated in the 3 subclasses. It should be abstracted. Move this logic to the base, chart class, as a new method. And call that method within all the sub classes. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:679: * Make GaLinChart a subclass of Chart class using chaining inheritance add period at end of line. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:736: * Make GaPieChart a subclass of Chart class using chaining inheritance add period at end of line https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:792: * Make GaBarChart a subclass of Chart class using chaining inheritance add period at end of line.
Sign in to reply to this message.
https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:624: * and maintain. I would delete the part "As a result the code is ..." https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:631: * attributes default setting specific to line charts. The 3 first parameters Delete the last line "The 3 first ..." https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:633: * A optianal configuration object is passed as a paramter and can override optional misspelled https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:633: * A optianal configuration object is passed as a paramter and can override s/A/An/ https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:635: * Essential default value for GaLineChart objects are: "Following default values are used for this object:" https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:637: * for the time: 'last-n-days': 30. s/time/date range/ https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:669: //if opt_config has no end-date, no start-date, and no last-n-days add space after // https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:688: * attributes default setting specific to pie charts. The 3 first parameters I think you can delete the last line "The 3 first ..." since these are covered under @param tags https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:689: * have for purpose to complete the default setting. I think you can delete the last last line "The first 3 parameters ..." since the params are covered under @param tags https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:690: * A optianal configuration object is passed as a paramter and can override An optional https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:692: * Essential default value for GaLineChart objects are: "Following default values are used for this object:" https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:745: * attributes default setting specific to line charts. The 3 first parameters Delete the last line "The 3 first ..." https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:747: * A optianal configuration object is passed as a paramter and can override An optional https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:749: * Essential default value for GaLineChart objects are: "Following default values are used for this object:"
Sign in to reply to this message.
All review notes has been addressed. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:631: * attributes default setting specific to line charts. The 3 first parameters On 2012/10/16 04:34:43, jsoneja wrote: > Delete the last line "The 3 first ..." Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:633: * A optianal configuration object is passed as a paramter and can override On 2012/10/16 04:34:43, jsoneja wrote: > optional misspelled Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:635: * Essential default value for GaLineChart objects are: On 2012/10/16 04:34:43, jsoneja wrote: > "Following default values are used for this object:" Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:637: * for the time: 'last-n-days': 30. On 2012/10/16 04:34:43, jsoneja wrote: > s/time/date range/ Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:670: if (!this.config.query['end-date'] && On 2012/10/09 17:43:40, nm wrote: > this is being replicated in the 3 subclasses. It should be abstracted. Move this > logic to the base, chart class, as a new method. And call that method within > all the sub classes. Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:679: * Make GaLinChart a subclass of Chart class using chaining inheritance On 2012/10/09 17:43:40, nm wrote: > add period at end of line. Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:688: * attributes default setting specific to pie charts. The 3 first parameters On 2012/10/16 04:34:43, jsoneja wrote: > I think you can delete the last line "The 3 first ..." since these are covered > under @param tags Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:689: * have for purpose to complete the default setting. On 2012/10/16 04:34:43, jsoneja wrote: > I think you can delete the last last line "The first 3 parameters ..." since the > params are covered under @param tags Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:690: * A optianal configuration object is passed as a paramter and can override On 2012/10/16 04:34:43, jsoneja wrote: > An optional Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:692: * Essential default value for GaLineChart objects are: On 2012/10/16 04:34:43, jsoneja wrote: > "Following default values are used for this object:" Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:736: * Make GaPieChart a subclass of Chart class using chaining inheritance On 2012/10/09 17:43:40, nm wrote: > add period at end of line Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:745: * attributes default setting specific to line charts. The 3 first parameters On 2012/10/16 04:34:43, jsoneja wrote: > Delete the last line "The 3 first ..." Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:747: * A optianal configuration object is passed as a paramter and can override On 2012/10/16 04:34:43, jsoneja wrote: > An optional Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:749: * Essential default value for GaLineChart objects are: On 2012/10/16 04:34:43, jsoneja wrote: > "Following default values are used for this object:" Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:792: * Make GaBarChart a subclass of Chart class using chaining inheritance On 2012/10/09 17:43:40, nm wrote: > add period at end of line. Done.
Sign in to reply to this message.
All changes has been uploaded https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:42: On 2012/10/09 17:43:40, nm wrote: > add a space between * and Namespace. Remove extra line. Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:48: * Boolean that checks to see if gapi client is loaded. On 2012/10/09 17:43:40, nm wrote: > space before *. Keep all * aligned, throughout this doc. Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:251: On 2012/10/09 17:43:40, nm wrote: > keep this comment Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:340: *with the GA response data. On 2012/10/09 17:43:40, nm wrote: > space between * & with Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:580: *http://perfectionkills.com/instanceof-considered-harmful-or-how-to-write-a-robust-isarray/ On 2012/10/09 17:43:40, nm wrote: > space after * Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:611: * Pie Chart On 2012/10/09 17:43:40, nm wrote: > remove this entire comment Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:618: * Laurent Jacquot, laurent1jacquot@gmail.com On 2012/10/09 17:43:40, nm wrote: > move this to top of file Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:621: * @fileoverview On 2012/10/09 17:43:40, nm wrote: > remove file overview. Just use a normal comment vs JSdoc...ie > > /* */ Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:622: * These added functions are designed to create an easier way to build chart On 2012/10/09 17:43:40, nm wrote: > remove added Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:623: * Gby wrapping the Chart object. As a result, the code is easier to develop On 2012/10/09 17:43:40, nm wrote: > Gby -> by Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:624: * and maintain. On 2012/10/16 04:34:43, jsoneja wrote: > I would delete the part "As a result the code is ..." Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:624: * and maintain. On 2012/10/16 04:34:43, jsoneja wrote: > I would delete the part "As a result the code is ..." Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:631: * attributes default setting specific to line charts. The 3 first parameters On 2012/10/16 04:34:43, jsoneja wrote: > Delete the last line "The 3 first ..." Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:631: * attributes default setting specific to line charts. The 3 first parameters On 2012/10/16 04:34:43, jsoneja wrote: > Delete the last line "The 3 first ..." Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:631: * attributes default setting specific to line charts. The 3 first parameters On 2012/10/16 04:34:43, jsoneja wrote: > Delete the last line "The 3 first ..." Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:633: * A optianal configuration object is passed as a paramter and can override On 2012/10/16 04:34:43, jsoneja wrote: > optional misspelled Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:633: * A optianal configuration object is passed as a paramter and can override On 2012/10/16 04:34:43, jsoneja wrote: > optional misspelled Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:633: * A optianal configuration object is passed as a paramter and can override On 2012/10/16 04:34:43, jsoneja wrote: > s/A/An/ Done. https://codereview.appspot.com/6625055/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:669: //if opt_config has no end-date, no start-date, and no last-n-days On 2012/10/16 04:34:43, jsoneja wrote: > add space after // Done.
Sign in to reply to this message.
https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:613: * Check the date of a wrapper s/Check/Checks/ https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:613: * Check the date of a wrapper add period at the end https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:622: chart.set({'last-n-days': 30}); This is inside an if-block, so please correct the indentation (should be +2 spaces inside the if-block) https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:628: * Line Chart Wrapper: remove ':' https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:629: * gadash.GaLineChart is a subclass of gadash.Chart add period https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:754: 'dimensions': 'ga:source' ga:source doesn't seem like a good default. Can we let user specify this along with the metric ? https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:776: * gadash.GaBarChart is a subclass of gadash.Chart add period at the end https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:824: * gadash.GaColumnChart is a subclass of gadash.Chart add period https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:853: 'chartOptions': { the chartOptions object is exactly the same in all the wrappers. Can you move this to the base class ?
Sign in to reply to this message.
Guys, You're almost done here. lets get these small fixes in and and finally submit this change. https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/7001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:754: 'dimensions': 'ga:source' On 2012/10/28 19:31:56, jsoneja wrote: > ga:source doesn't seem like a good default. Can we let user specify this along > with the metric ? I agree. How did you come up with source as a default? I also think this should be defined as a parameter in the constructor.
Sign in to reply to this message.
https://codereview.appspot.com/6625055/diff/13001/src/reporting/javascript/ez... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/13001/src/reporting/javascript/ez... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:805: 'chartOptions': { I think missed this comment from the last review. Can we either make this reusable by defining a default config or just move it to the base class ? We are repeating the same exact chart options in every wrapper.
Sign in to reply to this message.
https://codereview.appspot.com/6625055/diff/13001/src/reporting/javascript/ez... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/13001/src/reporting/javascript/ez... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:805: 'chartOptions': { As far as now, we have them different for visualization purposes, do you want us to integrate the new chartOptions for each chart with this API right now? On 2012/10/31 23:24:02, jsoneja wrote: > I think missed this comment from the last review. Can we either make this > reusable by defining a default config or just move it to the base class ? We are > repeating the same exact chart options in every wrapper.
Sign in to reply to this message.
LGTM. https://codereview.appspot.com/6625055/diff/13001/src/reporting/javascript/ez... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/13001/src/reporting/javascript/ez... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:805: 'chartOptions': { I think it would make sense to have a default chartOptions for each wrapper and then let the user override it, just like the config. For now though, you are right you could check this in and do it in a later changelist. Let's see what Nick says. He's out until Monday. On 2012/11/01 18:18:20, osama wrote: > As far as now, we have them different for visualization purposes, do you want us > to integrate the new chartOptions for each chart with this API right now? > > On 2012/10/31 23:24:02, jsoneja wrote: > > I think missed this comment from the last review. Can we either make this > > reusable by defining a default config or just move it to the base class ? We > are > > repeating the same exact chart options in every wrapper. >
Sign in to reply to this message.
https://codereview.appspot.com/6625055/diff/13001/src/reporting/javascript/ez... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6625055/diff/13001/src/reporting/javascript/ez... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:805: 'chartOptions': { hmm....I kinda agree.. ideally all of these configuration objects would live under some namespace like gadash.gviz.barChart = {....}; then the set method would only be this.set(gadash.gviz.barChart); That way people could reuse these configuration objects. I think if you then further abstracted out the common chartObject, it would require multiple set methods, which adds additional complexity to the API. this.set(gadash.gviz.barChart).set(gadash.gviz.defaultChart); So I'd recommend moving the chart objects to their own namespace, but keeping the default options. On 2012/11/01 22:45:30, jsoneja wrote: > I think it would make sense to have a default chartOptions for each wrapper and > then let the user override it, just like the config. > > For now though, you are right you could check this in and do it in a later > changelist. Let's see what Nick says. He's out until Monday. > > > On 2012/11/01 18:18:20, osama wrote: > > As far as now, we have them different for visualization purposes, do you want > us > > to integrate the new chartOptions for each chart with this API right now? > > > > On 2012/10/31 23:24:02, jsoneja wrote: > > > I think missed this comment from the last review. Can we either make this > > > reusable by defining a default config or just move it to the base class ? We > > are > > > repeating the same exact chart options in every wrapper. > > >
Sign in to reply to this message.
|
