Last active
December 17, 2015 04:09
-
-
Save ZER0/5548355 to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| let { Button } = require("sdk/ui"); | |
| let button = Button({ | |
| id: "my-button", | |
| // assuming automatically `data` folder for "./" | |
| image: "./beer.png", | |
| // required as tooltip and overflow | |
| label: "My Button", | |
| type: "checked" | |
| }); | |
| button.on("change", function (state) { | |
| // state is an immutable object that represent a snapshot | |
| // of the button's properties at the time the event was | |
| // fired. | |
| // It's the state of the button clicked by the user, in case | |
| // of base button we have 1 state per window. | |
| // returns the new state to merge with the current one | |
| return { image: state.checked ? "./coffee.png" : "./beer.png "}; | |
| }); | |
| // Let's say instead they want have a shared button: | |
| let { Button } = require("sdk/ui"); | |
| let sharedButton = Button({ | |
| id: "my-shared-button", | |
| image: "./beer.png", | |
| label: "My Shared Button", | |
| type: "checked" | |
| }); | |
| sharedButton.on("change", function (state) { | |
| // this will be propagated to all `sharedButton` states | |
| this.checked = state.checked; | |
| this.image = state.checked ? "./coffee.png" : "./beer.png "; | |
| }); | |
| // We could also provide low level utility function to get and set button state | |
| // outside our events, for instance: | |
| const tabs = require("sdk/tabs"); | |
| // The reason because I thought to functions instead of method, is mainly because | |
| // in most of use case add-on devs shouldn't have the needs to get this state, if we | |
| // provide the right set of events; so having them as low level API function makes | |
| // a clear separation between them and our high level API. | |
| const { getStateFor, setStateFor } = require("sdk/ui/utils"); | |
| // Note: this is something Location / Page Button would partially done internally | |
| // Here we handle the thing manually to show the concept | |
| tabs.on("activate", function (tab) { | |
| // get the state of mybutton in the "thing" given (can be both `tab` or `window`) | |
| let state = getStateFor(mybutton, tab); | |
| let isImageType = tab.contentType.indexOf("image/") === 0; | |
| let image = isImageType && state.checked ? "./on.png" : "./off.png"; | |
| // However, I don't really like this kind of function's signature, | |
| // I think could be improved | |
| setStateFor(mybutton, tab, {image: image}); | |
| }); | |
| // We can definitely improve that; but that's the basic idea | |
| // We don't have to think in term of "views" anymore, but in term of "state": | |
| // There are some conditions where we have 1 view and 1 state, but others | |
| // where we have 1 view and multiple states (e.g. Location Button) | |
| /** | |
| * Alternative approach #1 | |
| */ | |
| const { statesFor } = require("sdk/ui/utils"); | |
| tabs.on("activate", function (tab) { | |
| let states = statesFor(mybutton); // returns a 'list' of all states of this button | |
| // get the state of mybutton in the "thing" given (can be both `tab` or `window`) | |
| let state = states.get(tab); // maybe `get` is not the proper name | |
| let isImageType = tab.contentType.indexOf("image/") === 0; | |
| let image = isImageType && state.checked ? "./on.png" : "./off.png"; | |
| // maybe `set` is not the proper name | |
| states.set(tab, {image: image}); | |
| }); | |
| // The issue I see in these approaches is that `set` seems to intend to replace | |
| // entirely the previous status object, where is actually is `applied` on top of | |
| // it. That is done to avoid that the dev has to merge manually every time | |
| // the state object. Maybe another name could be better. | |
| /** | |
| * Alternative approach #2 | |
| */ | |
| tabs.on("activate", function (tab) { | |
| // get the state of mybutton in the "thing" given (can be both `tab` or `window`) | |
| let state = mybutton.stateFor(tab); | |
| let isImageType = tab.contentType.indexOf("image/") === 0; | |
| let image = isImageType && state.checked ? "./on.png" : "./off.png"; | |
| mybutton.stateFor(tab, {image: image}); | |
| }); | |
| /** | |
| * Alternative approach #3 | |
| */ | |
| tabs.on("activate", function (tab) { | |
| // get the 'button' of mybutton for given 'thing' (can be both `tab` or `window`) | |
| let button = mybutton.getButtonFor(tab); | |
| let isImageType = tab.contentType.indexOf("image/") === 0; | |
| let image = isImageType && button.checked ? "./on.png" : "./off.png"; | |
| button.image = image; | |
| }); | |
| /** | |
| * Alternative approach #4 | |
| * Basically instead properties it has method accessors. | |
| */ | |
| tabs.on("activate", function (tab) { | |
| let isImageType = tab.contentType.indexOf("image/") === 0; | |
| let image = isImageType && button.getChecked(tab) ? "./on.png" : "./off.png"; | |
| button.setImage(image, tab); | |
| }); |
I was having something more along this lines: https://gist.github.com/Gozala/5584873
Please note that while it may be little inconsistent with what we're doing now (although we don't really have context specific states or buttons yet) it greatly simplifies somewhat non trivial use cases like:
- Updating just a single button state scoped to specific UI component.
- Adding button to only specific set of windows.
- Making buttons with a shared state.
Matteo's examples make far more sense to me, with very little explanation I can understand how they work and what they will do. That is probably because they are more consistent with the rest of the SDK and every other API library I've ever used. We should build on that I think.
+1 for alt #2 not sure why "states" is plural though, how about X.getState(aThing?) ?
My bad, thanks to spot it out! So, good to hear we're all mostly on the same page; I was started already to implement a kind of approach #1 as low level API, in order to build the approach #2 on top of it.
Thanks to all you guys, I will update the Buttons JEP during the day!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I think I mostly lean towards alternative approach 2 because I think it's a common enough thing to want to do that you shouldn't need to import another module to do it. I think I'd probably be more explicit with the naming though, getState(tab) and setState(tab, state).
That isn't much of a strong argument though so if Irakli has stronger preferences then go with his choice.