Skip to main content

Search and Top Navigation

#4344 closed enhancement (fixed)

Opened March 16, 2009 01:06AM UTC

Closed August 31, 2010 02:19PM UTC

Last modified November 19, 2010 06:26PM UTC

Dialog: Enhanced Button Option

Reported by: scottgonzalez Owned by:
Priority: major Milestone: 1.8.5
Component: ui.dialog Version: 1.7
Keywords: Cc:
Blocked by: Blocking:
Description

Buttons need the following functionality:

  • ability to specify priority
  • ability to enable/disabled
  • ability to add/remove
  • ability to specify HTML, not just text, for the label

See jquery-ui-dev thread and jquery-ui-dev thread.

Attachments (2)
Change History (20)

Changed April 18, 2009 02:44AM UTC by scottgonzalez comment:1

Consider allowing an id to be specified for each button (see #4047).

Changed April 21, 2009 04:23PM UTC by andreas comment:2

Replying to [comment:1 scott.gonzalez]:

Consider allowing an id to be specified for each button (see #4047).

Yeah Id would be nice.

I hope it doesn't concern the jq ui devs that 1.7 is out with buttons not ordered in the same way they were added because fixing it will make it a backwards incompatible change.

Anyway is there someone working on this?

Changed April 22, 2009 02:44PM UTC by andreas comment:3

I added a svn diff for this. Not sure if thats the way to contribute with patches.

Anyway. The diff makes the following possible:

$('#foo').dialog({

'buttons' : {

'primary' : {

id:'fooButton',

priority: 'primary',

click: function() {

...

},

},

'secondary' : {

click: function() {

...

},

priority: 'secondary'

},

'disabled' : {

disabled: true

}

}

});

Im kind of a beginner at unit tests, I have checked the tests for dialog and I think I can manage to make tests for these changes BUT I wan't someone to review the patch to check if it's OK first and I also like the following to be discussed:

  • I realize the click callback is now optional. Is that OK?
  • If no id is set, the default '' will be used and the DOM will look like <button id="" ...>..</button> Is that OK? I think it's ugly but doing $('<button></button>').attr('id', options.id).prependTo(..) is so much nicer than var $button = $('<button></button>'); if(options.id !== '') { $button.attr('id', options.id) } $button.prependTo(..)

Differences between ui.dialog.diff and ui.dialog.diff.2

The first patch makes it possible to add custom css classes

$('#foo').dialog({

'buttons' : {

'cancel' : {

priority: 'secondary',

classes: 'foo bar baz',

click: function() {

...

},

},

}

});

Adding custom classes skips the ui-corner-all class. Example of usage: someone want's to make a cancel button look like a link instead of a rounded corner button. Though secondary priority and custom classes could be combined. Another example of usage is if someone would like only some of the corners rounded. The ui-corner-all class could be added with custom classes if needed.

The second patch takes another approach. Instead of allowing custom classes it takes advantage of the fact that you can put any string into priority. This applies to first patch aswell though.

Example:

$('#foo').dialog({

'buttons' : {

'foobar' : {

priority: 'custom-prio',

click: function() {

...

},

},

}

});

The button will then get a class of 'ui-priority-custom-prio'. To disable rounded corners with this patch one could use cornerAll: false

$('#foo').dialog({

'buttons' : {

'foobar' : {

priority: 'custom-prio',

cornerAll: false,

click: function() {

...

},

},

}

});

Im not sure either of this making it possible to customize the buttons is something wanted. It's probably very rare use-cases. I mean, either you use the jquery ui css framework or don't, not half of it.

Please discuss this and ideas and let me now how I can improve the patch.

Changed April 28, 2009 07:00PM UTC by andreas comment:4

Changed August 28, 2009 01:13AM UTC by scottgonzalez comment:5

description: Buttons need the following functionality:[[BR]] \ - ability to specify priority[[BR]] \ - ability to enable/disabled[[BR]] \ - ability to add/remove[[BR]] \ \ See [http://groups.google.com/group/jquery-ui-dev/browse_thread/thread/fa48f0eee0e27ee1 jquery-ui-dev thread].Buttons need the following functionality:[[BR]] \ - ability to specify priority[[BR]] \ - ability to enable/disabled[[BR]] \ - ability to add/remove[[BR]] \ \ See [http://groups.google.com/group/jquery-ui-dev/browse_thread/thread/fa48f0eee0e27ee1 jquery-ui-dev thread] and [http://groups.google.com/group/jquery-ui-dev/browse_thread/thread/4cb6ebd3b4bcd012 jquery-ui-dev thread].

Changed October 22, 2009 02:35AM UTC by scottgonzalez comment:6

description: Buttons need the following functionality:[[BR]] \ - ability to specify priority[[BR]] \ - ability to enable/disabled[[BR]] \ - ability to add/remove[[BR]] \ \ See [http://groups.google.com/group/jquery-ui-dev/browse_thread/thread/fa48f0eee0e27ee1 jquery-ui-dev thread] and [http://groups.google.com/group/jquery-ui-dev/browse_thread/thread/4cb6ebd3b4bcd012 jquery-ui-dev thread].Buttons need the following functionality:[[BR]] \ - ability to specify priority[[BR]] \ - ability to enable/disabled[[BR]] \ - ability to add/remove[[BR]] \ - ability to specify HTML, not just text, for the label[[BR]] \ \ See [http://groups.google.com/group/jquery-ui-dev/browse_thread/thread/fa48f0eee0e27ee1 jquery-ui-dev thread] and [http://groups.google.com/group/jquery-ui-dev/browse_thread/thread/4cb6ebd3b4bcd012 jquery-ui-dev thread].

Changed March 05, 2010 02:32PM UTC by scottgonzalez comment:7

See #5287 for a proposed patch (not reviewed).

Changed April 30, 2010 06:56PM UTC by rdworth comment:9

The current buttons implementation is extremely simple by design as it's a convenience. What about leaving it as is, and if you specify buttons: true you get the button pane, to which you can add your own buttons manually. I don't really see any end to the amount of access one might need to these dom elements (hover, focus, calling .button() on them, changing icons, hiding/showing, changing text, etc), and going through a dialog.buttons API to do all this just difficults things. It's not really a concern of the dialog how many buttons there are, in which order they're displayed, which is primary, etc. so it shouldn't get involved beyond creating a container for them.

Changed April 30, 2010 07:07PM UTC by AzaToth comment:10

Replying to [comment:11 rdworth]:

The current buttons implementation is extremely simple by design as it's a convenience. What about leaving it as is, and if you specify buttons: true you get the button pane, to which you can add your own buttons manually. I don't really see any end to the amount of access one might need to these dom elements (hover, focus, calling .button() on them, changing icons, hiding/showing, changing text, etc), and going through a dialog.buttons API to do all this just difficults things. It's not really a concern of the dialog how many buttons there are, in which order they're displayed, which is primary, etc. so it shouldn't get involved beyond creating a container for them.

The main reason for the change is to allow people to actually be able to set an id on the button without having to create the buttons manually.

Personally I'm against using labels as keys, as labels has the habit of being non-static (translations etc), and a design that is meant to only work with the most simplistic system seems to me to be the wrong way to do it.

Changed May 12, 2010 01:00PM UTC by jzaefferer comment:11

Replying to [comment:11 rdworth]:

The current buttons implementation is extremely simple by design as it's a convenience. What about leaving it as is, and if you specify buttons: true you get the button pane, to which you can add your own buttons manually. I don't really see any end to the amount of access one might need to these dom elements (hover, focus, calling .button() on them, changing icons, hiding/showing, changing text, etc), and going through a dialog.buttons API to do all this just difficults things. It's not really a concern of the dialog how many buttons there are, in which order they're displayed, which is primary, etc. so it shouldn't get involved beyond creating a container for them.

What does "you get the button pane" mean? Could you provide an example?

Changed May 12, 2010 01:12PM UTC by rdworth comment:12

Replying to [comment:12 AzaToth]:

Replying to [comment:11 rdworth]: > The current buttons implementation is extremely simple by design as it's a convenience. What about leaving it as is, and if you specify buttons: true you get the button pane, to which you can add your own buttons manually. I don't really see any end to the amount of access one might need to these dom elements (hover, focus, calling .button() on them, changing icons, hiding/showing, changing text, etc), and going through a dialog.buttons API to do all this just difficults things. It's not really a concern of the dialog how many buttons there are, in which order they're displayed, which is primary, etc. so it shouldn't get involved beyond creating a container for them. The main reason for the change is to allow people to actually be able to set an id on the button without having to create the buttons manually. Personally I'm against using labels as keys, as labels has the habit of being non-static (translations etc), and a design that is meant to only work with the most simplistic system seems to me to be the wrong way to do it.

You're arguing that we should do away with the simple system and replace it with a complex one because it doesn't meet the complex needs. I'm saying we should keep the simple system and not build in the the complex one at all, but have enough hooks that it can be added on. This is similar to the design of the autocomplete - it does basic local data sources out of the box. If you want to do remote data sources and caching, you're on your own, but the hooks are there to do it right in the right places.

Changed May 12, 2010 01:16PM UTC by rdworth comment:13

Replying to [comment:13 joern.zaefferer]:

Replying to [comment:11 rdworth]: > The current buttons implementation is extremely simple by design as it's a convenience. What about leaving it as is, and if you specify buttons: true you get the button pane, to which you can add your own buttons manually. I don't really see any end to the amount of access one might need to these dom elements (hover, focus, calling .button() on them, changing icons, hiding/showing, changing text, etc), and going through a dialog.buttons API to do all this just difficults things. It's not really a concern of the dialog how many buttons there are, in which order they're displayed, which is primary, etc. so it shouldn't get involved beyond creating a container for them. What does "you get the button pane" mean? Could you provide an example?

You would call

.dialog({ buttons: true })

and the dialog would generate the .ui-dialog-buttonpane element (as it does now if buttons is specified), to which you could manually append your manually created buttons.

.dialog("widget").find(".ui-dialog-buttonpane").append(...)

Changed May 16, 2010 09:38PM UTC by AzaToth comment:14

Replying to [comment:14 rdworth]:

You're arguing that we should do away with the simple system and replace it with a complex one because it doesn't meet the complex needs. I'm saying we should keep the simple system and not build in the the complex one at all, but have enough hooks that it can be added on. This is similar to the design of the autocomplete - it does basic local data sources out of the box. If you want to do remote data sources and caching, you're on your own, but the hooks are there to do it right in the right places.

The main issue here I say is that the current system makes it way to cumbersome if you just want to say add an class, use translated labels, add an image to an label etc... You shouldn't need to rewrite the logic of your code just to be able to do such things. And personally, I feel it's simpler to have static named labels, as it reduces the cognitive load to identify structures; True, I havn't don't any survey how it is out in the world regarding if current system, or my proposed system is best/easiest to use, so I have only my rather technical-wired mind to work with :).

Changed May 18, 2010 11:39AM UTC by ehynds comment:15

I think a good first step to improving the API is to create a new button instance and extend the button widget's options for each button. The API might change sightly, but would give you immediate access to all current (and future) options, and then some. It wouldn't be too difficult to retain backwards compatibility.

An API like this makes sense to me:

buttons: {
	"OK": {
		disabled: true, // available by extending the button widget
		icons: {}, // available by extending the button widget
		priority: 1,
		id: "ok-button",
		handler: function(){
			// do something on click
		}
	}
}

If you really wanted to interact with the buttons after initialization, let the button widget do the heavy lifting.

$("#button-ok").button("disable");

// or if you must, some kind of getter
$("#dialog").dialog("buttons", "button-ok").button("disable");

Allowing people to add and remove buttons after initialization seems like an edge case to me. Create all the buttons and either disable/enable/hide/show them based on actions taken inside the dialog.

Changed August 27, 2010 10:46PM UTC by scottgonzalez comment:16

I've got a pretty simple patch that adds as much flexibility as you want, but requires jQuery 1.4.

Diff: http://gist.github.com/554334

Demo: http://jsbin.com/aroje/edit

Changed August 28, 2010 12:07AM UTC by AzaToth comment:17

Replying to [comment:18 scott.gonzalez]:

I've got a pretty simple patch that adds as much flexibility as you want, but requires jQuery 1.4. Diff: http://gist.github.com/554334 Demo: http://jsbin.com/aroje/edit

/me likes

(didn't know .each() worked both for Array and for Object)

Changed August 31, 2010 02:19PM UTC by scottgonzalez comment:18

resolution: → fixed
status: newclosed

Fixed in 95a3459.

Changed September 10, 2010 05:24PM UTC by scottgonzalez comment:19

milestone: 1.91.8.5

Changed November 19, 2010 06:26PM UTC by Scott González comment:20

Dialog: Added additional syntax for creating buttons. Fixes #4344 - Dialog: Enhanced Button Option.

Changeset: 95a34593f98217e3a99f223e2ef3ca0a0fa8343b