Closed
Bug 951921
Opened 11 years ago
Closed 11 years ago
Document the new ActionButton and ToggleButton API
Categories
(Add-on SDK Graveyard :: Documentation, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zer0, Assigned: wbamberg)
References
Details
Attachments
(1 file)
We need to document the new Button API exposed by bug 951317
Priority: -- → P1
Assignee | ||
Comment 1•11 years ago
|
||
This is just for the action button. I suppose that for the toggle button we should duplicate this and just add the bit for "checked" and "change"?
Assignee: zer0 → wbamberg
Attachment #8361946 -
Flags: review?(zer0)
Reporter | ||
Comment 3•11 years ago
|
||
(In reply to Will Bamberg [:wbamberg] from comment #1)
> Created attachment 8361946 [details]
> mdn.html
>
> This is just for the action button. I suppose that for the toggle button we
> should duplicate this and just add the bit for "checked" and "change"?
You suppose well. We'll probably add some sugar thingy in the future, but at the moment that's it.
Also, I was wondering if you think the `onchange` event is just an overkill: I added because we have it in HTML/XUL checkbox's type of input, fired after `onclick`, but like in HTML in this specific case everything could be done by `click` event.
About the review, looks perfect! I would just maybe modify the example about using `state` with the button's instance itself, so instead passing just one property:
button.state(button, {
"label" : "button-global label"
});
I would set more than one, like `label` and `icon` for instance. So that it's explicitly clear what is the use case of passing `button`: instead of call every single property (button.label = ..., button.icon = ...) we could pass a set of property (e.g. like jquery) and devs can also keep this set as object (e.g. in `ToggleButton` having a `checkedStatus` object, that can be used for global or window / tab state) could be more useful.
I would also emphasize that the `state` passed by `click` and the `state` obtained by the namesake method, is a "snapshot": it means is an object with the button's property at the time where the button was clicked or the state was asked: to be clear, is not a living object: changing the button's property won't update the `state`:
button.label = 'foo';
let state = button.state(button);
button.label = 'bar';
console.log(state.label) // foo
That is made on purpose, in order to avoid racing issues where two part of code trying to update the same properties where another try to elaborate the state for example.
The documentation is already awesome, so I r+. If you agreed on those changes, add them too.
Reporter | ||
Updated•11 years ago
|
Attachment #8361946 -
Flags: review?(zer0) → review+
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3)
> (In reply to Will Bamberg [:wbamberg] from comment #1)
> > Created attachment 8361946 [details]
> > mdn.html
> >
> > This is just for the action button. I suppose that for the toggle button we
> > should duplicate this and just add the bit for "checked" and "change"?
>
> You suppose well. We'll probably add some sugar thingy in the future, but at
> the moment that's it.
> Also, I was wondering if you think the `onchange` event is just an overkill:
> I added because we have it in HTML/XUL checkbox's type of input, fired after
> `onclick`, but like in HTML in this specific case everything could be done
> by `click` event.
I think it's fine to have "change" as well, if that's what developers are used to seeing.
> About the review, looks perfect! I would just maybe modify the example about
> using `state` with the button's instance itself, so instead passing just one
> property:
>
> button.state(button, {
> "label" : "button-global label"
> });
>
> I would set more than one, like `label` and `icon` for instance. So that
> it's explicitly clear what is the use case of passing `button`: instead of
> call every single property (button.label = ..., button.icon = ...) we could
> pass a set of property (e.g. like jquery) and devs can also keep this set as
> object (e.g. in `ToggleButton` having a `checkedStatus` object, that can be
> used for global or window / tab state) could be more useful.
That makes sense to me. I did wonder what it was for.
Thanks Matteo!
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•