jquery-ui icon indicating copy to clipboard operation
jquery-ui copied to clipboard

Dialog > buttons option 'text' should be 'label' instead ?

Open pjfsilva opened this issue 3 years ago • 4 comments

When we are setting the buttons for a dialog, the option to set the button text is 'text': https://api.jqueryui.com/dialog/#option-buttons

But on the Button Widget the option is called 'label'. https://api.jqueryui.com/button/#option-label

To make it consistent between the two places, shouldn't the option for the dialog be also called label?

Looking at the code for dialog.js it seems the dialog never sets buttonOptions.label thus making it impossible to use the right option on the dialog.

I understand this breaks compatibility but it's pretty easy to keep retrocompatibility and offer the label option on dialog. It also makes it easier to understand the dialog docs if we support all the button options over there. (most are supported but not all)

pjfsilva avatar May 25 '22 03:05 pjfsilva

Thanks for the report. Does the issue you describe exist when jQuery UI 1.12.1 is used or only with jQuery UI 1.13.0 or newer?

mgol avatar May 25 '22 17:05 mgol

Yes, the inconsistency is the same on 1.12.1.

https://api.jqueryui.com/1.12/button/#option-label https://api.jqueryui.com/1.12/dialog/#option-buttons

pjfsilva avatar May 25 '22 20:05 pjfsilva

Thanks for the report. Since the issue is already in 1.12, given limited team resources it's not likely to be fixed by the UI team; see the project status at https://blog.jqueryui.com/2021/10/jquery-maintainers-update-and-transition-jquery-ui-as-part-of-overall-modernization-efforts/. PRs are welcome if they're not too complex. However, we should preserve backwards compatibility here.

@fnagel what do you think? Should we accept such a change if submitted?

mgol avatar Jun 01 '22 13:06 mgol

Sorry for the late response.

I agree that it should be possible to use the label and showLabel option when using the option buttons of the dialog widget. Actually only showLabel works but showText (as the docs say https://api.jqueryui.com/dialog/#option-buttons) does not. So the terminology is mixed up anyway. The source code looks like it was planned to remove the text option sometimes (see the Deprecated options" comments).

https://github.com/jquery/jquery-ui/blob/e21a2543b55680f23aaa7efa38f3288b8e767e7d/ui/widgets/dialog.js#L480

@mgol Yes for accepting a PR incl. support for dialog.buttons.label and dialog.buttons.showText for this from my side! We need to change the docs too.

fnagel avatar Jun 20 '22 21:06 fnagel