Merge lp:~edwin-grubbs/launchpad/bug-399554-timeline-improvements into lp:launchpad
- bug-399554-timeline-improvements
- Merge into devel
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Approved by: | Gavin Panella | ||||||||
Approved revision: | no longer in the source branch. | ||||||||
Merged at revision: | not available | ||||||||
Proposed branch: | lp:~edwin-grubbs/launchpad/bug-399554-timeline-improvements | ||||||||
Merge into: | lp:launchpad | ||||||||
Diff against target: |
758 lines 8 files modified
lib/canonical/launchpad/javascript/lp/dragscroll.js (+49/-8) lib/canonical/launchpad/javascript/registry/tests/timeline-iframe.html (+1/-1) lib/canonical/launchpad/javascript/registry/tests/timeline.js (+27/-8) lib/canonical/launchpad/javascript/registry/timeline.js (+210/-103) lib/lp/registry/model/product.py (+6/-3) lib/lp/registry/model/productseries.py (+1/-0) lib/lp/registry/stories/webservice/xx-project-registry.txt (+3/-0) lib/lp/registry/templates/timeline-macros.pt (+2/-1) |
||||||||
To merge this branch: | bzr merge lp:~edwin-grubbs/launchpad/bug-399554-timeline-improvements | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Albisetti (community) | ui | Approve | |
Michael Nelson (community) | ui* | Approve | |
Gavin Panella (community) | Approve | ||
Review via email: mp+12952@code.launchpad.net |
Commit message
Improve timeline graph by showing more information in the portlet on the project index page.
Description of the change
Edwin Grubbs (edwin-grubbs) wrote : | # |
Gavin Panella (allenap) wrote : | # |
> Line 133 character 9: The body of a for in should be wrapped in an if
> statement to filter unwanted properties from the prototype.
> for (i in this.series.
The error above is not very helpful, but the MDC does recommend against using for...in loops on Arrays:
"Although it may be tempting to use this as a way to iterate over an
Array, this is a bad idea. The for...in statement iterates over
user-defined properties in addition to the array elements"
-- https:/
I don't think you've added any properties to these arrays, but I think it's better to code defensively here. It'll also shut jslint up ;)
What do you think?
Gavin Panella (allenap) wrote : | # |
Hi Edwin,
This is a really useful change, and the code looks good :)
I have several questions and comments, so I'll mark it as Needs
Information, though some things might also need fixing, depending on
those questions. In any case, these are only minor things.
Gavin.
> === modified file 'lib/canonical/
> --- lib/canonical/
> +++ lib/canonical/
> @@ -55,7 +55,17 @@
> {name: '6', landmarks: [], is_development_
> {name: '7', landmarks: [], is_development_
> {name: '8', landmarks: [], is_development_
> - {name: '9', landmarks: [], is_development_
> + {name: '9', is_development_
> + landmarks: [
> + {
> + 'code_name': 'zamboni',
> + 'date': '2200-05-26',
> + 'name': 'ski',
> + 'type': 'milestone',
> + 'uri': 'file:/
> + }
> + ]
> + }
> ],
> resize_frame: 'timeline-iframe'
> };
> @@ -152,6 +162,15 @@
> this.timeline_
> },
>
> + test_milestone_
> + var label = this.content_
> + var second_line = label.query('div');
> + Assert.areEqual(
> + '2200-05-26',
> + second_
> + "Unexpected milestone date.");
> + },
> +
> test_resize_frame: function() {
> var frame = parent.
> this.timeline_
> @@ -166,7 +185,7 @@
> Assert.areEqual(1, this.timeline_
> Assert.areEqual(
> canvas.
> - 'The frame was not resized to match the canvas.');
> + '(1st) The frame was not resized to match the canvas.');
>
> simulate(
> this.timeline_
> @@ -177,7 +196,7 @@
> Assert.
> Assert.areEqual(
> canvas.
> - 'The frame was not resized to match the canvas.');
> + '(2nd) The frame was not resized to match the canvas.');
> Assert.isTrue(
> canvas.
> 'The canvas did not get scaled.');
> @@ -189,7 +208,7 @@
> Assert.areEqual(1, this.timeline_
> Assert.areEqual(
> canvas.
> - 'The frame was not resized to match the canvas.');
> + '(3rd) The frame was not resized to match the canvas.');
> }
> }));
>
> @@ -235,10 +254,10 @@
> "Unexpected milestone link href.");
>
> var second_line = label.query('div');
> - Assert.areEqual(
> - '2056-10-16...
Edwin Grubbs (edwin-grubbs) wrote : | # |
Hi Gavin,
Thanks for the review. An incremental diff is included at the bottom.
-Edwin
On Wed, Oct 7, 2009 at 8:20 AM, Gavin Panella
<email address hidden> wrote:
> Review: Needs Information code
> Hi Edwin,
>
> This is a really useful change, and the code looks good :)
>
> I have several questions and comments, so I'll mark it as Needs
> Information, though some things might also need fixing, depending on
> those questions. In any case, these are only minor things.
>
> Gavin.
>
>
>> === modified file 'lib/canonical/
>> --- lib/canonical/
>> +++ lib/canonical/
>> @@ -10,34 +10,33 @@
>>
>> var module = Y.namespace(
>>
>> -TIMELINE_GRAPH = 'timelinegraph';
>> +var TIMELINE_GRAPH = 'timelinegraph';
>> +var OBSOLETE_
>> var getCN = Y.ClassNameMana
>> var C_ZOOM_BOX = getCN(TIMELINE_
>> var C_ZOOM_IN = getCN(TIMELINE_
>> var C_ZOOM_OUT = getCN(TIMELINE_
>> var SECOND_MOUSE_BUTTON = 2;
>> // px spacing and sizes.
>> -var MARGIN_LEFT = 80;
>> +var MARGIN_LEFT = 20;
>> var MARGIN_TOP = 25;
>> var MARGIN_BOTTOM = 10;
>> -var Y_SPACING = 60;
>> -var LEGEND_INDENT= 90;
>> -var LEGEND_SPACING = 20;
>> -var LEGEND_BOX_PADDING = 7;
>> var MILESTONE_RADIUS = 5;
>> var RELEASE_RADIUS = 5;
>> var ARROW_HEIGHT = 10;
>> var ARROW_WIDTH = 15;
>> // Defines angle of vertical timeline.
>> -var ANGLE_X_SPACING = 0.3 * Y_SPACING;
>> +var ANGLE_DEGREES = 84;
>> +var ANGLE_RADIANS = ANGLE_DEGREES / 180 * Math.PI;
>> +var ANGLE_TANGENT = Math.tan(
>> // Font size in em's.
>> var FONT_SIZE = 1;
>> // Colors.
>> var LINE_COLOR = 'darkgreen';
>> +var OBSOLETE_
>> var MILESTONE_
>> var MILESTONE_
>> var RELEASE_COLOR = 'black';
>> -var LEGEND_BOX_COLOR = 'black';
>> var ARROW_COLOR = LINE_COLOR;
>> // Zoom level (increase/decrease 10%)
>> var ZOOM_JUMPS = 1.1;
>> @@ -96,7 +95,7 @@
>> this.timeline_graph = timeline_graph;
>> this.series = series;
>> this.start = start;
>> - var tooltip = 'Series';
>> + var tooltip = this.series.status + ' Series';
>> if (this.series.
>> tooltip = 'Development Focus Series';
>> }
>> @@ -105,18 +104,41 @@
>> this.series_label = this.timeline_
>> label_text, tooltip, this.series.uri,
>> {id: series.name});
>> +
>> this.labels = {};
>> for (var i in this.series.
>> - i = parseInt(i, 10);
>> var landmark = this.series.
>> var landmark_tooltip =
>> landmark.
>> if (Y.Lang.
>> landmark_tooltip += ': ' + landmark.code_name;
>> }
>> +
>> + var cfg = {id: landmark.name};
>> + if (Y.Lang.
>> + ...
Gavin Panella (allenap) wrote : | # |
On Thu, 08 Oct 2009 05:39:10 -0000
Edwin Grubbs <email address hidden> wrote:
> Hi Gavin,
>
> Thanks for the review. An incremental diff is included at the bottom.
The diff looks really good, and thanks for all your explanations :)
review approve
merge approve
> It took me quite a while to understand how evil prototype
> is. Ahhhhhh. I added an if-statement, although I think that there is
> a good chance that editing the prototype of built-in objects would
> actually cause the downfall of society, in which case, no bug would
> be reported.
:)
On one hand I quite like those libraries that monkeypatch
Array.prototype, String.prototype and others to add familiar and/or
useful methods. But it does make iterating through stuff so much more
difficult. Javascript would benefit from a better iterator protocol
(and, iirc, is getting one or has one in newer specs).
Edwin Grubbs (edwin-grubbs) wrote : | # |
Michael Nelson (michael.nelson) wrote : | # |
Wow - that is amazing Edwin! ui=me* with two suggestions/
In addition to the things you've mentioned, it also looks like you've also reduced the angle of the series line? Which means that it's a much better use of whitespace.
I see that you've ensured that the timeline always displays the current development focus tip by default? That's great - I wonder whether it would make sense to have the series name right-aligned along the milestone lines rather than centred? (ie. so it's always visible in the default project-index display such as shown at:
https:/
) Was there a reason for it being centred?
This is probably just related to the state of data on my local lp.dev, but after I ran the script to add the series and milestones, I expected all the 3-6 series to be above the 1.0 series - but they're not:
http://
perhaps it's because currently trunk (0.x) is the development focus? Ah yep, that did it - setting myseries-6 as the development focus (although 1.0 is still the next in the graph?).
Finally, regarding the 'grabability' mentioned in the bug - I wonder whether having separate icons for mouseover and mousedown would help? When I first played with it here, I wondered about the hand icon, clicked and nothing happened. If we had an open-hand-cursor during mouseover, and then when I click (more specifically, while the mouse button is down) the cursor changed to a fist - it might give people more visual feedback? (try clicking when viewing a photo in the default eye-of-gnome app).
Thanks for the excellent work - looking forward to seeing it on edge!
Martin Albisetti (beuno) wrote : | # |
Wonderful branch, and wonderful review.
Preview Diff
1 | === modified file 'lib/canonical/launchpad/javascript/lp/dragscroll.js' |
2 | --- lib/canonical/launchpad/javascript/lp/dragscroll.js 2009-06-10 04:06:59 +0000 |
3 | +++ lib/canonical/launchpad/javascript/lp/dragscroll.js 2009-10-09 03:26:12 +0000 |
4 | @@ -12,6 +12,7 @@ |
5 | DragScrollEventHandler = function() { |
6 | this.dragging = false; |
7 | this.last_position = null; |
8 | + this.event_listeners = []; |
9 | } |
10 | |
11 | DragScrollEventHandler.prototype = { |
12 | @@ -22,11 +23,11 @@ |
13 | * @method activate |
14 | */ |
15 | activate: function() { |
16 | - document.addEventListener("mousedown", this._startDragScroll, false); |
17 | - document.addEventListener("mouseup", this._stopDragScroll, false); |
18 | - document.addEventListener("mouseout", this._stopDragScroll, false); |
19 | - document.addEventListener("mousemove", this._dragScroll, false); |
20 | - document.body.style.cursor = 'move'; |
21 | + this._addEventListener("mousedown", this._startDragScroll); |
22 | + this._addEventListener("mouseup", this._stopDragScroll); |
23 | + this._addEventListener("mouseout", this._stopDragScroll); |
24 | + this._addEventListener("mousemove", this._dragScroll); |
25 | + this._setGrabCursor(); |
26 | }, |
27 | |
28 | /** |
29 | @@ -38,12 +39,50 @@ |
30 | deactivate: function() { |
31 | document.removeEventListener( |
32 | "mousedown", this._startDragScroll, false); |
33 | - document.removeEventListener("mouseup", this._stopDragScroll, false); |
34 | - document.removeEventListener("mouseout", this._stopDragScroll, false); |
35 | - document.removeEventListener("mousemove", this._dragScroll, false); |
36 | + this._removeEventListeners(); |
37 | + this._unsetCursor(); |
38 | + }, |
39 | + |
40 | + _addEventListener: function(event_type, action) { |
41 | + // Wrap the method in a different function that forces |
42 | + // `this` to be the `DragScrollEventHandler` object. |
43 | + var self = this; |
44 | + var event_listener = function(e) { |
45 | + action.call(self, e) |
46 | + }; |
47 | + var event_args = [event_type, event_listener, false]; |
48 | + this.event_listeners.push(event_args); |
49 | + document.addEventListener.apply(document, event_args); |
50 | + }, |
51 | + |
52 | + _removeEventListeners: function() { |
53 | + for (var i=0; i<this.event_listeners.length; i++) { |
54 | + var event_args = this.event_listeners[i]; |
55 | + document.removeEventListener.apply(document, event_args); |
56 | + } |
57 | + }, |
58 | + |
59 | + _unsetCursor: function() { |
60 | document.body.style.cursor = ''; |
61 | }, |
62 | |
63 | + _setGrabCursor: function() { |
64 | + // Styles for W3C, IE, Mozilla, Webkit. |
65 | + // Unknown styles will fail to change the value. |
66 | + document.body.style.cursor = 'move'; |
67 | + document.body.style.cursor = 'grab'; |
68 | + document.body.style.cursor = '-moz-grab'; |
69 | + document.body.style.cursor = '-webkit-grab'; |
70 | + }, |
71 | + |
72 | + _setGrabbingCursor: function() { |
73 | + // Styles for IE, Mozilla, and Webkit. |
74 | + // Unknown styles will fail to change the value. |
75 | + document.body.style.cursor = 'grabbing'; |
76 | + document.body.style.cursor = '-moz-grabbing'; |
77 | + document.body.style.cursor = '-webkit-grabbing'; |
78 | + }, |
79 | + |
80 | /** |
81 | * MouseDown event handler that causes _dragScroll to |
82 | * take action when it receives a MouseMove event. |
83 | @@ -54,6 +93,7 @@ |
84 | if (e.button == 0) { |
85 | this.dragging = true; |
86 | this.last_position = e; |
87 | + this._setGrabbingCursor(); |
88 | } |
89 | e.preventDefault(); |
90 | e.stopPropagation(); |
91 | @@ -69,6 +109,7 @@ |
92 | */ |
93 | _stopDragScroll: function(e) { |
94 | this.dragging = false; |
95 | + this._setGrabCursor(); |
96 | e.preventDefault(); |
97 | e.stopPropagation(); |
98 | }, |
99 | |
100 | === modified file 'lib/canonical/launchpad/javascript/registry/tests/timeline-iframe.html' |
101 | --- lib/canonical/launchpad/javascript/registry/tests/timeline-iframe.html 2009-07-01 02:25:33 +0000 |
102 | +++ lib/canonical/launchpad/javascript/registry/tests/timeline-iframe.html 2009-10-09 03:26:12 +0000 |
103 | @@ -17,7 +17,7 @@ |
104 | <style> |
105 | /* Taken and customized from testlogger.css */ |
106 | #log .yui-console-content { width:44em } |
107 | - #log .yui-console .yui-console-bd { height:30em } |
108 | + #log .yui-console .yui-console-bd { height:100%} |
109 | #log .yui-console .yui-console-controls { display:none; } |
110 | #log .yui-console .yui-console-hd { display:none; } |
111 | #log .yui-console .yui-console-ft { position:absolute;top:0em; } |
112 | |
113 | === modified file 'lib/canonical/launchpad/javascript/registry/tests/timeline.js' |
114 | --- lib/canonical/launchpad/javascript/registry/tests/timeline.js 2009-07-16 16:55:46 +0000 |
115 | +++ lib/canonical/launchpad/javascript/registry/tests/timeline.js 2009-10-09 03:26:12 +0000 |
116 | @@ -55,7 +55,17 @@ |
117 | {name: '6', landmarks: [], is_development_focus: false}, |
118 | {name: '7', landmarks: [], is_development_focus: false}, |
119 | {name: '8', landmarks: [], is_development_focus: false}, |
120 | - {name: '9', landmarks: [], is_development_focus: false} |
121 | + {name: '9', is_development_focus: false, |
122 | + landmarks: [ |
123 | + { |
124 | + 'code_name': 'zamboni', |
125 | + 'date': '2200-05-26', |
126 | + 'name': 'ski', |
127 | + 'type': 'milestone', |
128 | + 'uri': 'file:///firefox/+milestone/alpha' |
129 | + } |
130 | + ] |
131 | + } |
132 | ], |
133 | resize_frame: 'timeline-iframe' |
134 | }; |
135 | @@ -152,6 +162,15 @@ |
136 | this.timeline_graph.destroy(); |
137 | }, |
138 | |
139 | + test_milestone_label_second_line: function() { |
140 | + var label = this.content_box.query('div#ski'); |
141 | + var second_line = label.query('div'); |
142 | + Assert.areEqual( |
143 | + '2200-05-26', |
144 | + second_line.get('innerHTML'), |
145 | + "Unexpected milestone date."); |
146 | + }, |
147 | + |
148 | test_resize_frame: function() { |
149 | var frame = parent.document.getElementById( |
150 | this.timeline_graph.resize_frame); |
151 | @@ -166,7 +185,7 @@ |
152 | Assert.areEqual(1, this.timeline_graph.graph_scale); |
153 | Assert.areEqual( |
154 | canvas.get('offsetHeight'), frame.height, |
155 | - 'The frame was not resized to match the canvas.'); |
156 | + '(1st) The frame was not resized to match the canvas.'); |
157 | |
158 | simulate( |
159 | this.timeline_graph, '.yui-timelinegraph-zoom-in', 'click'); |
160 | @@ -177,7 +196,7 @@ |
161 | Assert.areEqual(1.1, this.timeline_graph.graph_scale); |
162 | Assert.areEqual( |
163 | canvas.get('offsetHeight'), frame.height, |
164 | - 'The frame was not resized to match the canvas.'); |
165 | + '(2nd) The frame was not resized to match the canvas.'); |
166 | Assert.isTrue( |
167 | canvas.get('offsetHeight') > first_canvas_height, |
168 | 'The canvas did not get scaled.'); |
169 | @@ -189,7 +208,7 @@ |
170 | Assert.areEqual(1, this.timeline_graph.graph_scale); |
171 | Assert.areEqual( |
172 | canvas.get('offsetHeight'), frame.height, |
173 | - 'The frame was not resized to match the canvas.'); |
174 | + '(3rd) The frame was not resized to match the canvas.'); |
175 | } |
176 | })); |
177 | |
178 | @@ -235,10 +254,10 @@ |
179 | "Unexpected milestone link href."); |
180 | |
181 | var second_line = label.query('div'); |
182 | - Assert.areEqual( |
183 | - '2056-10-16', |
184 | - second_line.get('innerHTML'), |
185 | - "Unexpected milestone date."); |
186 | + Assert.isNull( |
187 | + second_line, |
188 | + "There should be no second line for landmarks when " + |
189 | + "resize_frame is false."); |
190 | } |
191 | })); |
192 | |
193 | |
194 | === modified file 'lib/canonical/launchpad/javascript/registry/timeline.js' |
195 | --- lib/canonical/launchpad/javascript/registry/timeline.js 2009-07-27 14:27:53 +0000 |
196 | +++ lib/canonical/launchpad/javascript/registry/timeline.js 2009-10-09 03:26:12 +0000 |
197 | @@ -10,34 +10,33 @@ |
198 | |
199 | var module = Y.namespace('registry.timeline'); |
200 | |
201 | -TIMELINE_GRAPH = 'timelinegraph'; |
202 | +var TIMELINE_GRAPH = 'timelinegraph'; |
203 | +var OBSOLETE_SERIES_STATUS = 'Obsolete'; |
204 | var getCN = Y.ClassNameManager.getClassName; |
205 | var C_ZOOM_BOX = getCN(TIMELINE_GRAPH, 'zoom-box'); |
206 | var C_ZOOM_IN = getCN(TIMELINE_GRAPH, 'zoom-in'); |
207 | var C_ZOOM_OUT = getCN(TIMELINE_GRAPH, 'zoom-out'); |
208 | var SECOND_MOUSE_BUTTON = 2; |
209 | // px spacing and sizes. |
210 | -var MARGIN_LEFT = 80; |
211 | +var MARGIN_LEFT = 20; |
212 | var MARGIN_TOP = 25; |
213 | var MARGIN_BOTTOM = 10; |
214 | -var Y_SPACING = 60; |
215 | -var LEGEND_INDENT= 90; |
216 | -var LEGEND_SPACING = 20; |
217 | -var LEGEND_BOX_PADDING = 7; |
218 | var MILESTONE_RADIUS = 5; |
219 | var RELEASE_RADIUS = 5; |
220 | var ARROW_HEIGHT = 10; |
221 | var ARROW_WIDTH = 15; |
222 | // Defines angle of vertical timeline. |
223 | -var ANGLE_X_SPACING = 0.3 * Y_SPACING; |
224 | +var ANGLE_DEGREES = 84; |
225 | +var ANGLE_RADIANS = ANGLE_DEGREES / 180 * Math.PI; |
226 | +var ANGLE_TANGENT = Math.tan(ANGLE_RADIANS); |
227 | // Font size in em's. |
228 | var FONT_SIZE = 1; |
229 | // Colors. |
230 | var LINE_COLOR = 'darkgreen'; |
231 | +var OBSOLETE_SERIES_COLOR = '#777777'; |
232 | var MILESTONE_LINE_COLOR = 'darkgray'; |
233 | var MILESTONE_FILL_COLOR = 'white'; |
234 | var RELEASE_COLOR = 'black'; |
235 | -var LEGEND_BOX_COLOR = 'black'; |
236 | var ARROW_COLOR = LINE_COLOR; |
237 | // Zoom level (increase/decrease 10%) |
238 | var ZOOM_JUMPS = 1.1; |
239 | @@ -50,15 +49,10 @@ |
240 | */ |
241 | var draw_line = function(canvas_context, points, fill) { |
242 | canvas_context.beginPath(); |
243 | - for (i in points) { |
244 | - if (i === 0) { |
245 | - // Starting the line. |
246 | - canvas_context.moveTo(points[i].x, points[i].y); |
247 | - } else { |
248 | - // Specify points. |
249 | - canvas_context.lineTo(points[i].x, points[i].y); |
250 | - } |
251 | - } |
252 | + canvas_context.moveTo(points[0].x, points[0].y); |
253 | + Y.each(points.slice(1), function(point, i) { |
254 | + canvas_context.lineTo(point.x, point.y); |
255 | + }); |
256 | // Draw! |
257 | if (fill === true) { |
258 | canvas_context.fill(); |
259 | @@ -96,28 +90,59 @@ |
260 | this.timeline_graph = timeline_graph; |
261 | this.series = series; |
262 | this.start = start; |
263 | - var tooltip = 'Series'; |
264 | + var tooltip = this.series.status + ' Series'; |
265 | if (this.series.is_development_focus) { |
266 | tooltip = 'Development Focus Series'; |
267 | } |
268 | - var label_text = Y.Node.create('<strong/>'); |
269 | - label_text.appendChild(document.createTextNode(series.name)); |
270 | - this.series_label = this.timeline_graph.make_label( |
271 | - label_text, tooltip, this.series.uri, |
272 | - {id: series.name}); |
273 | + |
274 | this.labels = {}; |
275 | - for (var i in this.series.landmarks) { |
276 | - i = parseInt(i, 10); |
277 | - var landmark = this.series.landmarks[i]; |
278 | + Y.each(this.series.landmarks, function(landmark, i) { |
279 | var landmark_tooltip = |
280 | landmark.type.charAt(0).toUpperCase() + landmark.type.substr(1); |
281 | if (Y.Lang.isString(landmark.code_name)) { |
282 | landmark_tooltip += ': ' + landmark.code_name; |
283 | } |
284 | + |
285 | + var cfg = {id: landmark.name}; |
286 | + if (Y.Lang.isString(this.timeline_graph.resize_frame)) { |
287 | + cfg.second_line = landmark.date; |
288 | + } |
289 | this.labels[landmark.name] = this.timeline_graph.make_label( |
290 | - landmark.name, landmark_tooltip, landmark.uri, |
291 | - {second_line: landmark.date, id: landmark.name}); |
292 | + landmark.name, landmark_tooltip, landmark.uri, cfg); |
293 | + }, this); |
294 | + |
295 | + // If the frame is not going to be resized, the dates are |
296 | + // not displayed under the landmarks, so a single date |
297 | + // is displayed at the end of the series line where it |
298 | + // will not increase the vertical spacing. |
299 | + this.series_date_label = null; |
300 | + if (!Y.Lang.isString(this.timeline_graph.resize_frame)) { |
301 | + for (var i=0; i < this.series.landmarks.length; i++) { |
302 | + var landmark = this.series.landmarks[i]; |
303 | + if (landmark.date !== null) { |
304 | + this.series_date_label = this.timeline_graph.make_label( |
305 | + '', 'Last date in series', this.series.uri, |
306 | + {second_line: landmark.date, |
307 | + id: series.name + '-' + landmark.date}); |
308 | + break; |
309 | + } |
310 | + } |
311 | } |
312 | + |
313 | + // Center series label. |
314 | + var label_text = Y.Node.create('<strong/>'); |
315 | + label_text.appendChild(document.createTextNode(series.name)); |
316 | + this.center_series_label = this.timeline_graph.make_label( |
317 | + label_text, tooltip, this.series.uri, |
318 | + {id: series.name}); |
319 | + // Left label. |
320 | + this.left_series_label = this.timeline_graph.make_label( |
321 | + '', tooltip, this.series.uri, |
322 | + {second_line: series.name, id: series.name}); |
323 | + // Right label. |
324 | + this.right_series_label = this.timeline_graph.make_label( |
325 | + '', tooltip, this.series.uri, |
326 | + {second_line: series.name, id: series.name}); |
327 | }; |
328 | |
329 | SeriesLine.prototype = { |
330 | @@ -128,10 +153,36 @@ |
331 | * @method get_length |
332 | */ |
333 | get_length: function() { |
334 | + // No arrow at the end of obsolete series lines. |
335 | + var length = 0; |
336 | + if (this.series.status == OBSOLETE_SERIES_STATUS) { |
337 | + length = this.series.landmarks.length * |
338 | + this.timeline_graph.landmark_spacing; |
339 | + } else { |
340 | + length = (this.series.landmarks.length + 1) * |
341 | + this.timeline_graph.landmark_spacing; |
342 | + } |
343 | // Display a line stub for series without any landmarks. |
344 | - return (this.series.landmarks.length + 1) * |
345 | - this.timeline_graph.landmark_spacing; |
346 | - }, |
347 | + return Math.max(length, this.timeline_graph.min_series_line_length); |
348 | + }, |
349 | + |
350 | + /** |
351 | + * Calculate the vertical spacing of the horizontal lines based on twice |
352 | + * the height of the series name, plus the height of the landmark text, |
353 | + * which may or may not have a second line for the date. |
354 | + * |
355 | + * @method get_y_spacing() |
356 | + */ |
357 | + get_y_spacing: function() { |
358 | + var max_y = 0; |
359 | + Y.each(this.series.landmarks, function(landmark, i) { |
360 | + var label = this.labels[landmark.name]; |
361 | + max_y = Math.max(label.get('offsetHeight')); |
362 | + }, this); |
363 | + return max_y + (2 * RELEASE_RADIUS) + |
364 | + this.center_series_label.get('offsetHeight'); |
365 | + }, |
366 | + |
367 | |
368 | /** |
369 | * The main method which is called by the ProjectLine.draw() |
370 | @@ -146,38 +197,74 @@ |
371 | this.start.x + this.get_length(), |
372 | this.start.y); |
373 | |
374 | - if (this.series.is_development_focus === true) { |
375 | - // Draw a thick line as a rectangle. |
376 | - var thickness = 4; |
377 | - var offset = -2; |
378 | - context.fillStyle = LINE_COLOR; |
379 | - context.fillRect( |
380 | - this.start.x, |
381 | - this.start.y + offset, |
382 | - stop.x - this.start.x, |
383 | - stop.y - this.start.y + thickness); |
384 | - } |
385 | - else { |
386 | - // We can't draw a 1 pixel wide rectangle reliably, so |
387 | - // we have to use the line drawing method. |
388 | - context.strokeStyle = LINE_COLOR; |
389 | - draw_line(context, [this.start, stop]); |
390 | - } |
391 | + var thickness, offset; |
392 | + // Draw a line of various thicknesses as a rectangle. |
393 | + if (this.series.status == OBSOLETE_SERIES_STATUS) { |
394 | + thickness = 2; |
395 | + offset = -1; |
396 | + context.fillStyle = OBSOLETE_SERIES_COLOR; |
397 | + } else if (this.series.is_development_focus) { |
398 | + thickness = 4; |
399 | + offset = -2; |
400 | + context.fillStyle = LINE_COLOR; |
401 | + } else { |
402 | + thickness = 1; |
403 | + offset = 0; |
404 | + context.fillStyle = LINE_COLOR; |
405 | + } |
406 | + context.fillRect( |
407 | + this.start.x, |
408 | + this.start.y + offset, |
409 | + stop.x - this.start.x, |
410 | + stop.y - this.start.y + thickness); |
411 | |
412 | // Arrow at end of series line. |
413 | - this.timeline_graph.make_landmark(stop, 'arrow'); |
414 | + if (this.series.status != OBSOLETE_SERIES_STATUS) { |
415 | + this.timeline_graph.make_landmark(stop, 'arrow'); |
416 | + } |
417 | |
418 | - // Series label. |
419 | - var label_position = new Position( |
420 | + // Center series label. |
421 | + var center_position = new Position( |
422 | this.start.x + (this.get_length() / 2), |
423 | this.start.y - RELEASE_RADIUS); |
424 | this.timeline_graph.place_label( |
425 | - label_position, this.series_label, 'center', 'above'); |
426 | + center_position, this.center_series_label, 'center', 'above'); |
427 | + |
428 | + |
429 | + // Only show the left and right series labels if the |
430 | + // series line is wider than the viewport (iframe). |
431 | + var line_width = this.get_length() * this.timeline_graph.graph_scale; |
432 | + if (line_width < Y.DOM.winWidth()) { |
433 | + this.left_series_label.setStyle('display', 'none'); |
434 | + this.right_series_label.setStyle('display', 'none'); |
435 | + } else { |
436 | + this.left_series_label.setStyle('display', 'block'); |
437 | + this.right_series_label.setStyle('display', 'block'); |
438 | + |
439 | + // Left series label. |
440 | + var left_position = new Position( |
441 | + this.start.x + 10, |
442 | + this.start.y - RELEASE_RADIUS); |
443 | + this.timeline_graph.place_label( |
444 | + left_position, this.left_series_label, 'right', 'above'); |
445 | + |
446 | + // Right series label. |
447 | + var right_position = new Position( |
448 | + this.start.x + this.get_length(), |
449 | + this.start.y - RELEASE_RADIUS); |
450 | + this.timeline_graph.place_label( |
451 | + right_position, this.right_series_label, 'left', 'above'); |
452 | + } |
453 | + |
454 | + if (this.series_date_label !== null) { |
455 | + var label_position = new Position( |
456 | + stop.x + (ARROW_WIDTH / 2), this.start.y); |
457 | + this.timeline_graph.place_label( |
458 | + label_position, this.series_date_label, 'right', 'middle'); |
459 | + } |
460 | |
461 | // Landmark labels. |
462 | - for (var i in this.series.landmarks) { |
463 | - i = parseInt(i, 10); |
464 | - var landmark = this.series.landmarks[i]; |
465 | + Y.each(this.series.landmarks, function(landmark, i) { |
466 | // The newest milestones are at the beginning, and |
467 | // they need to be placed at the end of the horizontal |
468 | // line. |
469 | @@ -196,7 +283,7 @@ |
470 | this.timeline_graph.place_label( |
471 | landmark_label_position, this.labels[landmark.name], |
472 | 'center', 'below'); |
473 | - } |
474 | + }, this); |
475 | } |
476 | }; |
477 | |
478 | @@ -209,19 +296,18 @@ |
479 | * @constructor |
480 | */ |
481 | ProjectLine = function(timeline_graph, timeline) { |
482 | + if (timeline.length === 0) { |
483 | + throw new Error("The timeline array is empty."); |
484 | + } |
485 | this.timeline_graph = timeline_graph; |
486 | this.timeline = timeline; |
487 | |
488 | - var width = (this.timeline.length - 1) * ANGLE_X_SPACING; |
489 | - this.start = new Position( |
490 | - MARGIN_LEFT, |
491 | - (this.timeline.length-1) * Y_SPACING + MARGIN_TOP); |
492 | - this.stop = new Position( |
493 | - this.start.x + width, |
494 | - MARGIN_TOP); |
495 | - |
496 | this.series_lines = []; |
497 | this.initSeries(); |
498 | + |
499 | + this.start = this.series_lines[0].start.copy(); |
500 | + var last_series = this.series_lines[this.series_lines.length-1]; |
501 | + this.stop = last_series.start.copy(); |
502 | }; |
503 | |
504 | ProjectLine.prototype = { |
505 | @@ -235,19 +321,23 @@ |
506 | * @method initSeries |
507 | */ |
508 | initSeries: function() { |
509 | - for (var i in this.timeline) { |
510 | - // Convert index to an actual integer so it can |
511 | - // used in calculations. |
512 | - i = parseInt(i, 10); |
513 | - var series = this.timeline[i]; |
514 | - |
515 | - var series_position = new Position( |
516 | - this.start.x + (i * ANGLE_X_SPACING), |
517 | - this.start.y - (i * Y_SPACING)); |
518 | - |
519 | + var current = new Position(0, MARGIN_TOP); |
520 | + var reversed_timeline = this.timeline.slice().reverse(); |
521 | + Y.each(reversed_timeline, function(series, i) { |
522 | var series_line = new SeriesLine( |
523 | - this.timeline_graph, series, series_position); |
524 | + this.timeline_graph, series, current.copy()); |
525 | this.series_lines.push(series_line); |
526 | + |
527 | + var height = series_line.get_y_spacing(); |
528 | + current.x -= height / ANGLE_TANGENT; |
529 | + current.y += height; |
530 | + }, this); |
531 | + |
532 | + if (current.x < MARGIN_LEFT) { |
533 | + var shift_x = -current.x + MARGIN_LEFT; |
534 | + Y.each(this.series_lines, function(series_line, i) { |
535 | + series_line.start.x += shift_x; |
536 | + }, this); |
537 | } |
538 | }, |
539 | |
540 | @@ -260,8 +350,8 @@ |
541 | */ |
542 | get_width: function() { |
543 | var max_x = 0; |
544 | - for (var i in this.series_lines) { |
545 | - var landmarks = this.series_lines[i].series.landmarks; |
546 | + Y.each(this.series_lines, function(series_line, i) { |
547 | + var landmarks = series_line.series.landmarks; |
548 | var text_beyond_last_landmark; |
549 | if (landmarks.length === 0) { |
550 | // Even a project with zero landmarks needs to have |
551 | @@ -277,12 +367,12 @@ |
552 | this.series_lines[i].get_length() + |
553 | text_beyond_last_landmark; |
554 | max_x = Math.max(max_x, series_width); |
555 | - } |
556 | + }, this); |
557 | return max_x; |
558 | }, |
559 | |
560 | /** |
561 | - * Calculate the height based on the start.y value, which |
562 | + * Calculate the height based on the stop.y value, which |
563 | * is based on the number of series. It also adds the |
564 | * distance for the labels below the bottom series line. |
565 | * |
566 | @@ -293,11 +383,13 @@ |
567 | var bottom_label_height = 0; |
568 | var last_series = this.series_lines[this.series_lines.length-1]; |
569 | for (var key in last_series.labels) { |
570 | - var label = last_series.labels[key]; |
571 | - bottom_label_height = Math.max( |
572 | - bottom_label_height, label.get('offsetHeight')); |
573 | + if (last_series.labels.hasOwnProperty(key)) { |
574 | + var label = last_series.labels[key]; |
575 | + bottom_label_height = Math.max( |
576 | + bottom_label_height, label.get('offsetHeight')); |
577 | + } |
578 | } |
579 | - return this.start.y + bottom_label_height + RELEASE_RADIUS; |
580 | + return this.stop.y + bottom_label_height + RELEASE_RADIUS; |
581 | }, |
582 | |
583 | /** |
584 | @@ -309,9 +401,9 @@ |
585 | var context = this.timeline_graph.canvas_context; |
586 | context.strokeStyle = LINE_COLOR; |
587 | draw_line(context, [this.start, this.stop]); |
588 | - for (var i in this.series_lines) { |
589 | - this.series_lines[i].draw(); |
590 | - } |
591 | + Y.each(this.series_lines, function(series_line, i) { |
592 | + series_line.draw(); |
593 | + }, this); |
594 | } |
595 | }; |
596 | |
597 | @@ -391,10 +483,17 @@ |
598 | * @method recreate_canvas |
599 | */ |
600 | recreate_canvas: function() { |
601 | - var width = this.graph_scale * |
602 | - (this.project_line.get_width() + MARGIN_LEFT); |
603 | - var height = this.graph_scale * this.project_line.get_height() + |
604 | - MARGIN_BOTTOM; |
605 | + var width = Math.ceil( |
606 | + this.graph_scale * (this.project_line.get_width() + MARGIN_LEFT)); |
607 | + |
608 | + // The get_height() method already includes the MARGIN_TOP, so that |
609 | + // gets multiplied by the graph_scale. Alternatively, we could have |
610 | + // made changes elsewhere so that MARGIN_LEFT and MARGIN_TOP are |
611 | + // not scaled at all. |
612 | + var height = Math.ceil( |
613 | + (this.graph_scale * this.project_line.get_height()) + |
614 | + MARGIN_BOTTOM); |
615 | + |
616 | if (this.canvas) { |
617 | this.get('contentBox').removeChild(this.canvas); |
618 | } |
619 | @@ -425,20 +524,21 @@ |
620 | */ |
621 | calculate_landmark_spacing: function() { |
622 | var max_label_width = 0; |
623 | - for (var i in this.project_line.series_lines) { |
624 | - var series_line = this.project_line.series_lines[i]; |
625 | - max_label_width = Math.max( |
626 | - max_label_width, series_line.series_label.get('offsetWidth')); |
627 | - for (var j in series_line.labels) { |
628 | - var label = series_line.labels[j]; |
629 | + var series_max_label_width = 0; |
630 | + Y.each(this.project_line.series_lines, function(series_line, i) { |
631 | + series_max_label_width = Math.max( |
632 | + series_max_label_width, |
633 | + series_line.center_series_label.get('offsetWidth')); |
634 | + Y.each(series_line.labels, function(label, j) { |
635 | // We have to set the font size here so that |
636 | // offsetWidth will be correct. |
637 | this.set_font_size(label); |
638 | max_label_width = Math.max( |
639 | max_label_width, label.get('offsetWidth')); |
640 | - } |
641 | - } |
642 | + }, this); |
643 | + }, this); |
644 | this.landmark_spacing = max_label_width + 5; |
645 | + this.min_series_line_length = series_max_label_width + 5; |
646 | }, |
647 | |
648 | /** |
649 | @@ -459,14 +559,19 @@ |
650 | * @method scroll_to_last_development_focus_landmark |
651 | */ |
652 | scroll_to_last_development_focus_landmark: function(label) { |
653 | - var series_lines = this.project_line.series_lines; |
654 | - var series_line = series_lines[series_lines.length-1]; |
655 | + var series_line = this.project_line.series_lines[0]; |
656 | var landmark = series_line.series.landmarks[0]; |
657 | if (landmark) { |
658 | var landmark_label = series_line.labels[landmark.name]; |
659 | - var scroll_x = landmark_label.get('offsetLeft') + |
660 | - landmark_label.get('offsetWidth') - |
661 | - window.getViewportDimensions().w; |
662 | + var date_label_width = 0; |
663 | + if (series_line.series_date_label !== null) { |
664 | + date_label_width = |
665 | + series_line.series_date_label.get('offsetWidth'); |
666 | + } |
667 | + var scroll_x = |
668 | + series_line.start.x + series_line.get_length() + |
669 | + ARROW_WIDTH + date_label_width - |
670 | + Y.DOM.winWidth(); |
671 | // scrollBy is relative, so adjust it by |
672 | // the current scroll position. |
673 | scroll_x -= window.scrollX; |
674 | @@ -619,6 +724,8 @@ |
675 | y_align_offset = -label_height; |
676 | } else if (y_align == 'below') { |
677 | y_align_offset = 0; |
678 | + } else if (y_align == 'middle') { |
679 | + y_align_offset = -(label_height / 2); |
680 | } else { |
681 | throw "Invalid y_align argument: " + y_align; |
682 | } |
683 | |
684 | === modified file 'lib/lp/registry/model/product.py' |
685 | --- lib/lp/registry/model/product.py 2009-10-01 16:19:10 +0000 |
686 | +++ lib/lp/registry/model/product.py 2009-10-09 03:26:12 +0000 |
687 | @@ -996,9 +996,12 @@ |
688 | series_list.remove(self.development_focus) |
689 | series_list.insert(0, self.development_focus) |
690 | series_list.reverse() |
691 | - return [series.getTimeline(include_inactive=include_inactive) |
692 | - for series in series_list |
693 | - if include_inactive or series.active] |
694 | + return [ |
695 | + series.getTimeline(include_inactive=include_inactive) |
696 | + for series in series_list |
697 | + if include_inactive or series.active or |
698 | + series == self.development_focus |
699 | + ] |
700 | |
701 | |
702 | class ProductSet: |
703 | |
704 | === modified file 'lib/lp/registry/model/productseries.py' |
705 | --- lib/lp/registry/model/productseries.py 2009-09-16 21:22:12 +0000 |
706 | +++ lib/lp/registry/model/productseries.py 2009-10-09 03:26:12 +0000 |
707 | @@ -569,6 +569,7 @@ |
708 | return dict( |
709 | name=self.name, |
710 | is_development_focus=self.is_development_focus, |
711 | + status=self.status.title, |
712 | uri=canonical_url(self, path_only_if_possible=True), |
713 | landmarks=landmarks) |
714 | |
715 | |
716 | === modified file 'lib/lp/registry/stories/webservice/xx-project-registry.txt' |
717 | --- lib/lp/registry/stories/webservice/xx-project-registry.txt 2009-10-01 12:30:32 +0000 |
718 | +++ lib/lp/registry/stories/webservice/xx-project-registry.txt 2009-10-09 03:26:12 +0000 |
719 | @@ -430,6 +430,7 @@ |
720 | u'type': u'release', |
721 | u'uri': u'/firefox/1.0/1.0.0'}], |
722 | u'name': u'1.0', |
723 | + u'status': u'Active Development', |
724 | u'uri': u'/firefox/1.0'}, |
725 | {u'is_development_focus': True, |
726 | u'landmarks': [{u'code_name': None, |
727 | @@ -453,6 +454,7 @@ |
728 | u'type': u'release', |
729 | u'uri': u'/firefox/trunk/0.9'}], |
730 | u'name': u'trunk', |
731 | + u'status': u'Obsolete', |
732 | u'uri': u'/firefox/trunk'}] |
733 | |
734 | |
735 | @@ -820,6 +822,7 @@ |
736 | {u'is_development_focus': False, |
737 | u'landmarks': [], |
738 | u'name': u'foobadoo', |
739 | + u'status': u'Active Development', |
740 | u'uri': u'/babadoo/foobadoo'} |
741 | |
742 | |
743 | |
744 | === modified file 'lib/lp/registry/templates/timeline-macros.pt' |
745 | --- lib/lp/registry/templates/timeline-macros.pt 2009-07-17 18:46:25 +0000 |
746 | +++ lib/lp/registry/templates/timeline-macros.pt 2009-10-09 03:26:12 +0000 |
747 | @@ -21,9 +21,10 @@ |
748 | |
749 | <!-- Opera ignores overflow:hidden for iframe, so use scrolling=no. --> |
750 | <iframe id="timeline-iframe" name='timeline-iframe' |
751 | + class="timeline-iframe" |
752 | style="display: none; border: 0" |
753 | scrolling="no" |
754 | - width="100%" height="116px"></iframe> |
755 | + width="100%" height="216px"></iframe> |
756 | <script> |
757 | function timeline_iframe(auto_resize, include_inactive) { |
758 | var timeline_url = "+timeline-graph?"; |
Summary
-------
Improves the timeline graph portlet on the project index page by allowing
more information to be seen without scrolling.
Implementation details ------- ------- -
-------
Hid dates for milestones/releases when showing the graph on the project index
page. Moved horizontal series lines closer together. Added a date at the end of
the series line, if at least one of the milestones or releases has a date.
Removed the arrow from obsolete series and made its horizontal line gray.
Tests
-----
file:// /./lib/ canonical/ launchpad/ javascript/ registry/ tests/timeline. html lib/canonical/ launchpad/ windmill/ tests/test_ registry/ test_timeline_ graph.py firefox http:// launchpad. dev:8085
./bin/lp-windmill test=./
Demo and Q/A
------------
* Open http:// launchpad. dev/firefox
* The timeline graph portlet should be bigger and only show dates at the
end of the line.
Lint
----
I fixed all the lint errors except these rediculous ones regarding for-loops.
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files: /launchpad/ javascript/ registry/ tests/timeline. js /launchpad/ javascript/ registry/ timeline. js registry/ model/productse ries.py registry/ templates/ timeline- macros. pt
lib/canonical
lib/canonical
lib/lp/
lib/lp/
== JSLint notices == egrubbs/ canonical/ lp-branches/ checkout/ lib/canonical/ launchpad/ javascript/ registry/ timeline. js': landmarks) {
jslint: Lint found in '/home/
Line 110 character 5: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
for (var i in this.series.
Line 133 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype. landmarks) {
for (i in this.series.
Line 174 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype. landmarks) {
for (var i in this.series.
Line 238 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype. landmarks) {
for (var i in this.series.
Line 296 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
for (var i in reversed_timeline) {
Line 310 character 13: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
for (i in this.series_lines) {
Line 325 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
for (var i in this.series_lines) {
Line 357 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
for (var key in last_series.labels) {
Line 374 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
for (var i in this.series_lines) {
Line 499 character 13: The body of a for in should be wrapped in an if statement to filter unwanted properties f...