johnspackman
commented
8 years ago I agree, I think we've all probably got our own dialog classes and it's an addition that's long overdue.
I have some suggestions however:
at https://github.com/qooxdoo/qooxdoo/compare/master...yybalam:dialoglab#diff-db36f17ac6773546a7d9d40012eb8b0dR96 there is no need for aspect.icon, because it's duplicating what is available by:
new qx.ui.dialog.Confirm(...).set({ icon: "icon" });
Rather than buttons property being a string of "yes", "yesno", "yesnocancel" etc, why not make it an array - this makes it possible to expect the dialog and add new button types (it would simplify __composeButtons also)
__getButtons should be private to allow new button types in derived classes
The design is to provide a map of button IDs to labels (or labels and icons) in aspectOptons and a separate map of button IDs to callbacks in handlers, and a context which is in that handlers map.
IMHO that spreads the detail of each button around in two or three different places and will make the code confusing; it is more "qooxdoo-ish" to have a single event of (eg) "buttonClicked" and provide the button ID (ie "ok"/"cancel"/etc). It is useful to provide a callback and context to the constructor so that these can be attached automatically.
new qx.ui.dialog.Confirm("Title", "Message", [ "ok", "cancel" ], function(){}, this);or with more control over button properties:
new qx.ui.dialog.Confirm("Title", "Message", [
{ id: "ok", label: "Accept", icon: "icons/myicon.png" },
{ id: "cancel", label: "No Thanks", icon: "icons/myicon.png" },
], function(){}, this);If you want to have individual callbacks per button, then combine them in the button array, eg:
new qx.ui.dialog.Confirm("Title", "Message", [
{ id: "ok", label: "Accept", icon: "icons/myicon.png", callback: function(){}, context: this },
{ id: "cancel", label: "No Thanks", icon: "icons/myicon.png", callback: function(){}, context: this },
]);Where the handlers property has a global context defined, this should be set on the dialog itself rather than embedded in one of the configuration maps, eg the example above could become:
new qx.ui.dialog.Confirm("Title", "Message", [
{ id: "ok", label: "Accept", icon: "icons/myicon.png", callback: function(){} },
{ id: "cancel", label: "No Thanks", icon: "icons/myicon.png", callback: function(){} },
]).set({ icon: "icons/mydialog.png", context: this });
As others JS libraries, will be nice and useful if qooxdoo provide an own set of dialog widgets. Actually there is a lot a dialog implementations, some as contrib, some inside projects, etc.
As a lab I create on my fork a branch with a possible proposal of how I think can be qx dialogs. At moment I think is some premature make a PR, but also want to do the proposal.
https://github.com/qooxdoo/qooxdoo/compare/master...yybalam:dialoglab
Also I create a little demo app: http://yybalam.net/qxlab/dialoglab/
If you think that my proposal have future I will be glad on take tips, fix the bugs still remain on my lab, and add tests and documentation.