Callback issue with confirm

[henryruhs]

Hello,

the confirm example from the website is working well. Removing the failure callback causes everything to get mixed up. Pressing "cancel" shows the success note and pressing "ok" closes the dialog.

Code:

alertify.confirm('Confirm Title', 'Confirm Message', function() { alertify.success('Ok') });

Demo:
http://jsfiddle.net/bm85j26k/

Workaround
Adding null for the failure callback works around the issue, but this is still a bug from my point of view.

[MohammadYounes]

Hi,

This is a misuse rather than a bug, The 3rd overload has the following signature:

/*
 * @message {String or DOMElement} The dialog contents.
 * @onok {Function} Invoked when the user clicks OK button.
 * @oncancel {Function} Invoked when the user clicks Cancel button or closes the dialog.
 *
 *  alertify.confirm(message, onok, oncancel);
 *
 */
alertify.confirm(
  'Confirm Message', 
  function(){ alertify.success('Ok') }, 
  function(){ alertify.error('Cancel')}
);

You are explicitly setting onok callback value to "Confirm Message" and oncancel value to function() { alertify.success('Ok') }.

Thanks.

[henryruhs]

This must be a joke - this is 100% a misbehaviour as callbacks should be optional...

[MohammadYounes]

They are optional in the first overload

/*
 * @message {String or DOMElement} The dialog contents.
 *
 *  alertify.confirm(message);
 *
 */
alertify.confirm('Confirm Message');

[henryruhs]

So you have optional parameter followed by required parameter... find the error

However, it is upon you - I will not be the last one who traps into this unusual API design.

Best regards

[MohammadYounes]

So you have optional parameter followed by required parameter

There is no optional parameter followed by a required one, The API is designed based on the most common use-cases.

1. Message:
   alertify.confirm(message);
2. Message and Confirmation callback:
   alertify.confirm(message, onok);
3. Message, Confirmation and Cancellation callbacks:
   alertify.confirm(message, onok, oncancel);
4. Title, Message, Confirmation and Cancellation callbacks:
   alertify.confirm(title, message, onok, oncancel);

I don't see how alertify.confirm(title, message, oncancel); should be a default overload?

Even if the overloads were designed to adhere to parameters types (not count) - it would make more sense for a confirm dialog to have alertify.confirm(title, message, onok); instead. As the user will be more interested in the action confirmation rather than its cancellation.

Thanks.

[henryruhs]

alertify.confirm(title, message, onok) is what I was talking about all the time. Have you tried the jsfiddle link? I don't care about cancle handling, the dialog should just close and that's it.

[MohammadYounes]

Yes, I did.

You are explicitly setting onok callback value to "Confirm Message" and oncancel value to function() { alertify.success('Ok') }.

I am a bit unfocused now :)

I don't see how alertify.confirm(title, message, oncancel); should be a default overload?

That being said, the current overloads are based on parameters count (not type). Taking into account that title is available in the defaults, Which will be more common alertify.confirm(message, onok, oncancel); or alertify.confirm(title, message, onok); ?

Anyhow, the library is very extensible and provides you with the ability to create your own confirm dialog or inherit the existing one.

alertify.dialog('myconfirm', function() {
  return {
    main: function(title, message, onok) {
      this.set('title', title);
      this.set('message', message);
      this.set('onok', onok);
      return this
    }
  }
},false, 'confirm')


alertify.myconfirm('Confirm Title', 'Confirm Message', function() {
  alertify.success('Ok')
});

http://jsfiddle.net/nzsekuco/

[henryruhs]

Thanks god, we finally speak the same language^^ ... sorry for being a bit annoying

I would just allow alertify.confirm(title, message, onok) as alertify.confirm(message, onok, oncancel) would be a breaking change!? That would be a nice move for an 1.11.3 release.

May I suggest Promise for an upcoming 2.0.0 release?

alertify
.confirm({
    title: 'Confirm Title',
    message: 'Confirm Message'
})
.then(() => alertify.success('Ok')
.catch(() => alertify.error('Ok')
});

[MohammadYounes]

I would just allow alertify.confirm(title, message, onok) as alertify.confirm(message, onok, oncancel) would be a breaking change!?

Yes, this is will be a breaking change and can't be done until 2.0.0

As for promises, it will be there once I find time for 2.0.0 release (see #126)

[henryruhs]

Okay, so if you release an 1.11.3 I would be really happy.

Let me know if you need help with 2.0.0 as of experience in TypeScript, Unit Testing or just Code Reviews. Drop my a line via mail anytime.

[MohammadYounes]

Okay, so if you release an 1.11.3 I would be really happy.

You lost me here, changing the API can't be done before 2.0

Let me know if you need help with 2.0.0

Thank you! My main problem I can't find time to work on this.

Ever thought of splitting up the builds ...

My plan for 2.0 is to be extremely modular, not just the notifications but also move, resize ...etc.

[henryruhs]

Okay, sounds good - let me know when you have time.

[IvanJasenov]

In this demo, at https://alertifyjs.com/confirm/basic.html

alertify.alert()
 .setting({
   'label':'Agree',
   'message': 'This dialog is : ' + (closable ? ' ' : ' not ') + 'closable.' ,
   'onok': function(){ alertify.success('Great');}
 }).show();

Is there a way to execute another function for 'onclick' property? I cannot execute a callback function which will be fired once the confirmation is clicked. I'm working in Angular 8.

[MohammadYounes]

@IvanJasenov Not sure I understand your request, If you mean 'onok' ? Why not to invoke them in the same callback ?

alertify.alert()
 .setting({
   'label':'Agree',
   'message': 'This dialog is : ' + (closable ? ' ' : ' not ') + 'closable.' ,
   'onok': function(){ 
        // do action 1
        alertify.success('Great');
        // do action 2
         .... 
        // do action ... n
   }
 }).show();

If you mean 'oncancel', you can add them to the passed settings object:

alertify.alert()
 .setting({
   'label':'Agree',
   'message': 'This dialog is : ' + (closable ? ' ' : ' not ') + 'closable.' ,
   'onok': function(){  alertify.success('Great'); },
   'oncancel': ...,
   'onclose': ... , 
   'onclosing: ... , 
 }).show();

[IvanJasenov]

Hi, @MohammadYounes
Yes, I tried

'onok': function(){ 
        this.saySomething()
   }

but once I referenced a function inside like this.saySomething() it raises an error. Even just a simple function with no input params. On the other hand I tried
'onok': this.callServiceMethod(param1, param2)
and this one works fine.

[MohammadYounes]

@IvanJasenov I think this is a matter of context, try:

//save current context
var self = this;

alertify.alert()
 .setting({
   'label':'Agree',
   'message': 'This dialog is : ' + (closable ? ' ' : ' not ') + 'closable.' ,
   'onok': function(){ 
        // do action 1 using the saved context
        self.callServiceMethod(p1,p2);
        // do action 2
         .... 
        // do action ... n
   }
 }).show();