951921 - Document the new ActionButton and ToggleButton API

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: Documentation, defect, P1)

Description

[Matteo Ferretti [:zer0] [:matteo]]

We need to document the new Button API exposed by bug 951317

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: 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"?

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

(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.

Comment 4

[Will Bamberg [:wbamberg]]

(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!