bootboxjs / bootbox

Wrappers for JavaScript alert(), confirm() and other flexible dialogs using Twitter's bootstrap framework
http://bootboxjs.com
Other
5.04k stars 1.04k forks source link

Support specifying id attribute for button objects #812

Closed ronan-paul closed 1 year ago

ronan-paul commented 2 years ago

Hello

Having an id attribute for button objects is a major need for test automation (which is why this new request duplicates #598 which has been closed).

Here is why it is needed

So I would propose this to be implemented with either:

Option 1 - create an optional id property for each button

bootbox.dialog({ 
    title: 'FullScreen ',
    message: '<p>Do you want to go for fullscreen</p>',
    buttons: {
        cancel: {
            label: 'Oh no',
            id: 'dialog-fullscreen-btn-cancel', //new property
            className: 'btn-primary',
            callback: function(){
                  //whatever
            }
        },
        accept: {
            label: 'Go for it',
            id: 'dialog-fullscreen-btn-accept', //new property
            className: 'btn-info', 
            callback: function(){
                  //whatever
            }
        }
    }
})

Option 2 - use button key as id property (less compliant with current semantic of bootbox IMO)

bootbox.dialog({ 
    title: 'FullScreen ',
    message: '<p>Do you want to go for fullscreen</p>',
    buttons: {
        dialog-fullscreen-btn-cancel: {
            label: 'Oh no',
            className: 'btn-primary',
            callback: function(){
                  //whatever
            }
        },
        dialog-fullscreen-btn-accept: {
            label: 'Go for it',
            className: 'btn-info', 
            callback: function(){
                  //whatever
            }
        }
    }
})

Any version would create these 2 buttons :

<button id="dialog-fullscreen-btn-cancel" type="button" class="btn btn-primary">Oh no</button>
<button id="dialog-fullscreen-btn-accept" type="button" class="btn btn-info">Go for it</button>

This improvement would allow automation tests to run with specific selector like:

$I->click('#dialog-fullscreen-btn-accept');

and no longer:

$I->click('button[title="Go for it"]');

or

$I->click('button.my-button-class');

Thank you in advance

tiesont commented 2 years ago

This is supported in v6, which is a work in progress - https://github.com/makeusabrew/bootbox/blob/v6-wip/bootbox.js#L305

I won't be backporting this to v5, so it's up to you how comfortable you are with testing a WIP/beta version.

ronan-paul commented 2 years ago

@tiesont sounds great !

I am open to WIP... but may I ask how advanced is this v6 ?

PS: I think it would also be good to have an id set for the modal itself, with the same goal and benefits.

tiesont commented 2 years ago

@tiesont sounds great !

I am open to WIP... but may I ask how advanced is this v6 ?

PS: I think it would also be good to have an id set for the modal itself, with the same goal and benefits.

The test suite has been updated to test the modals against both BS4 and BS5, and everything passes, so it's probably safe to try updating your code. I just haven't really exercised the v6 version with much hands-on testing yet, hence why it's still labeled as a WIP.

ronan-paul commented 2 years ago

The test suite has been updated to test the modals against both BS4 and BS5, and everything passes, so it's probably safe to try updating your code. I just haven't really exercised the v6 version with much hands-on testing yet, hence why it's still labeled as a WIP.

Is it also still BS3 compatible ?

tiesont commented 2 years ago

Most likely, yes. Modal markup hasn't changed too much. The main issue is that not all options will work, but that's always been the case.

I might try adding tests for BS3, but it depends on free time and motivation, so who knows.

ronan-paul commented 2 years ago

The test suite has been updated to test the modals against both BS4 and BS5, and everything passes, so it's probably safe to try updating your code. I just haven't really exercised the v6 version with much hands-on testing yet, hence why it's still labeled as a WIP.

Is it also still BS3 compatible ?

Hello,

I did include the latest bootbox.js (v6-wip) in my project which runs on BS3.3. Everything looks quite smooth and working. Kudos.

A few remarks:

  1. I tried to add a default value for size in the defaults object, but it has not been taken into account.
  2. Any "medium" size available ?
  3. I wish I could also set an id for the dialog, so that I can also check if a specific dialog is being shown (whatever its label are). => Is that also possible to implement ?

I will run some additional tests and give you feedback of course.

Regards,

Ronan

tiesont commented 2 years ago

Any "medium" size available ?

No. "Medium" would be a normal-sized modal, so that size would serve no purpose - hence why it doesn't exist in Bootstrap. The size options are all used for setting these.

I wish I could also set an id for the dialog

I'd rather not. There is both an onShow and onShown option for which you can set a callback function.

I'll look into the size thing - if that isn't working, then it never has. I don't remember if we ever intended to, but size was a relatively late addition to the codebase.

ronan-paul commented 2 years ago

Hey

Thank you for your answer and precisions.

Let me just underline that the main need for Ids is interface test automation, in which you need to use selectors to identify elements without ambiguity (but you can not interact with JavaScript by adding code, so callbacks can not be used).

With this angle, would you consider this addition as a valid need ?

Regards

Ronan

Ronan PAUL 07 87 85 42 56

Le 7 juin 2022 à 22:17, Tieson Trowbridge @.***> a écrit :

 Any "medium" size available ?

No. "Medium" would be a normal-sized modal, so that size would serve no purpose - hence why it doesn't exist in Bootstrap.

I wish I could also set an id for the dialog

I'd rather not. There is both an onShow and onShown option for which you can set a callback function.

I'll look into the size thing - if that isn't working, then it never has. I don't remember if we ever intended to, but size was a relatively late addition to the codebase.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

tiesont commented 2 years ago

I suppose I can look into it. I make no promises, though.

tiesont commented 1 year ago

Closed in #820

txwizard commented 1 month ago

In addition to test automation, adding ID tags throughout expands significantly the ability to add other features, such as tolltips.