Hi Larry, On Fri, Oct 24, 2008 at 2:09 AM, LarryK <[EMAIL PROTECTED]> wrote: > > Hi Adrien, > > I just ran the first parts of your patch against the timeline.js file > > Several issues: > 1) You're not sending the theme to LinearEther
Oops, I think I cleaned too much the code before submitting the patch. That's definitely an error. > 2) You shouldn't use startsOn and endsOn to control "beginningOfTime" > and "endOfTime." The reason is that startsOn and endsOn variables are > already in use (as you note), but for something completely different: > they are used to move the left side or right side of an ether to a > specific date. So to avoid confusion, use different variable names, eg > "beginningOfTime" and "endOfTime." Ok. Will do. > 2) You haven't defined startsOn or endsOn if they're missing from the > incoming params. Perhaps this was intentional, but if so, it usually > isn't a good idea to have to deal with missing keys unless really > necessary. Why not define them with value null to indicate that they > haven't been set? It also enables the code to be shorter. I will define them with default null values. > 3) Too many nested if statements. Since LinearEther is already taking > an object, not positional args, it is clear to define a static object > rather than creating a new variable (etherParams) and a bunch of if > statements. > 4) The Javascript conditional operator is cleaner than ifs, see below. I don't like the ternary operator :-). But I will use it in the timeline code. Just for my personal culture : is this a pure convention or is this operator supposed to be more efficient ? > 5) Regarding key centersOn: Your logic does not include the key if > startsOn or endsOn is defined. Same issues of missing keys as above, I > suggest that you always include the key. Your logic in the Ether code > will ignore centersOn if startsOn or endsOn is defined. (Be sure to > document the usage of the keys as appropriate.) Will do. > As mentioned above, since you are adding more keys to LinearEther, you > should take this opportunity to document ALL of them. The > documentation should be added to the definition of LinearEther, not to > where it is called. If my code is of any interest, I will of course document new keys. > Another issue is that I suggest that rather than subclassing the _band > object, you may just want to add more logic to its methods that need > to be changed. > Or, for bands that have beginningOfTime/endOfTime dates: after the > band object has been instantiated, just replace the couple of methods > that are different. Eg I presume the _moveEther would change. I liked the subclassing approach. If my code doesn't get in the trunk, I would prefer not to have to modify the official timeline code directly. More generally, I find a bit hard to extend the existing code for a custom usage without modifying the timeline code directly or use monkey patching. For example, in case I want to add a custom date range in my application, I see no way to do this without modifying the code of roundDownToInterval and incremenetInterval of SimileAjax.DateTime. Did I miss something on that specific point ? That said, I realize I don't come with a solution :-). > My thought on adding beginningOfTime/endOfTime dates is that the > Ether would simply ignore _moveEther requests that would result in > showing dates earlier that beginningOfTime (when moving back in time) > or later than endOfTime when moving forward in time. Note that you > should allow for the corner case of a *wide* monitor that would have > the beginningOfTime on the left side of the screen and a date AFTER > the endOfTime on the right side. You could also solve by having the > etherPainter show a neutral gray before the beginningOfTime and after > the endOfTime. Actually, what I would like (maybe it's not a good idea and counter arguments are welcomed) is to _always_ start at "beggingOnTime" and end at "endOfTime", whatever the screen size is. In that case, the time unit would be "dynamic". For example, say the width is 3000px, and the time range is 150 years, I _think_ I would like the timeline to consider my "intervalUnitPixels", say 100px, and do the rest of the job and automatically adapt the time unit (with a potential rounding to have nice dates). In that case, the time unit would be 5 years. Am I clear enough ? Is it a bad idea ? > > My suggestion for the next step is that you prepare all of your > changes as one checkin so they can be evaluated as a whole. And please > also create a demonstration / test file. Will do. I won't have access to a computer until monday, but I will do that next week. > Thanks again for your time with the project. Thanks for your time and your precious feedback. Cheers, --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "SIMILE Widgets" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/simile-widgets?hl=en -~----------~----~----~----~------~----~------~--~---
