Closed Bug 585390 Opened 14 years ago Closed 14 years ago

add panel property to Widget API

Categories

(Add-on SDK Graveyard :: General, enhancement)

Product:
Add-on SDK Graveyard
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(1 file)

patch v1: implements enhancement
7.11 KB, patch
dietrich
: review+
Details | Diff | Splinter Review 
The Widget API should have a panel property that simplifies the common case of a widget that displays a panel when you click on it.

This bug is for the "panel property of Widget API" portion of the all-in-one patch in bug 494238, which I'm breaking into smaller pieces to make it easier to review.
Attachment #463869 - Flags: review?(dietrich)
Comment on attachment 463869 [details] [diff] [review]
patch v1: implements enhancement

>+    <td><tt>panel</tt></td>
>+    <td>
>+      A Panel to open when the user clicks on the widget.  See the `panel`
>+      module for more information about the `Panel` objects to which this
>+      option can be set and the `reddit-panel` example add-on for an example
>+      of using this option.
>+      
>+      Note: if you also specify an `onClick` callback function, it will be
>+      called instead of the panel being opened.
>+    </td>
>+  </tr>

maybe add a note explaining that it's still possible to open the panel manually if you have a custom click handler.

>+    panel: {
>+      is: ["null", "undefined", "object"],
>+    },

should check that it's actually a panel object.

>@@ -197,16 +206,18 @@ let browserManager = {
>   // all currently registered windows, and when new windows are registered it
>   // will be added to them, too.
>   addItem: function browserManager_addItem(item) {
>     let idx = this.items.indexOf(item);
>     if (idx > -1)
>       throw new Error("The widget " + item + " has already been added.");
>     this.items.push(item);
>     this.windows.forEach(function (w) w.addItems([item]));
>+    if (item.panel)
>+      panels.add(item.panel);

will the panel module add panels to each open window? if so, this could be done just once, in the widget ctor.

(hm, i should've reviewed the panels core patch first!)

>@@ -217,16 +228,18 @@ let browserManager = {
>   // of all windows that are currently registered.
>   removeItem: function browserManager_removeItem(item) {
>     let idx = this.items.indexOf(item);
>     if (idx == -1) {
>       throw new Error("The widget " + item + " has not been added " +
>                       "and therefore cannot be removed.");
>     }
>     this.items.splice(idx, 1);
>+    if (item.panel)
>+      panels.remove(item.panel);

similar to the previous comment - if panel handles all windows, then this should move to the widget dtor.

>+exports.testPanelWidget = function testPanelWidget(test) {
>+  let widgets = require("widget");
>+  let widget = widgets.Widget({
>+    label: "panel widget",
>+    content: "<div id='me'>foo</div>",
>+    onLoad: function(e) {
>+      sendMouseEvent({type:"click"}, "me", e.target.defaultView);
>+    },
>+    panel: require("panel").Panel({
>+      content: "data:text/html,<body>Look ma, a panel!</body>",
>+      onShow: function() {
>+        widgets.remove(widget);
>+        test.pass("panel displayed on click");
>+        test.done();
>+      }
>+    })
>+  });
>+  widgets.add(widget);
>+  test.waitUntilDone();
>+};

should add a test for when there's both a panel and a click handler defined.

r=me with comments addressed.
Attachment #463869 - Flags: review?(dietrich) → review+
Note: I pulled the changes for this back into the patch for bug 494238, whose example depends on it.  So once that bug is resolved, this one can be too.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: