jameskleeh / angular-confirm

Confirmation modal dialog for AngularJS
Apache License 2.0
150 stars 75 forks source link

Feature to allow custom bootstrap classes for the OK/Cancel buttons #69

Closed stguitar closed 8 years ago

stguitar commented 8 years ago

We sometimes use confirm dialogs in more of a "negative" context, where dismissing the dialog warrants a 'danger' class for the button.

I have created the ability to pass in the desired bootstrap button class to the directive for each of the OK and Cancel buttons.

I have pull requested this, but let me know if this isnt the process that you prefer, or if you want to ignore it entirely.

Thanks!

jameskleeh commented 8 years ago

The issue with this is I don't want to add options that only work in the context of bootstrap css. Some users don't use bootstrap. So you would have to specify all of the classes: btn btn-warning instead of just warning. Just giving the ability to specify additional classes wouldn't work for your use case.

Perhaps the ability to create named templates would be better. The name would map to a templateUrl or template string defined ahead of time. Then you can completely customize it and easily reference it. Thoughts?

$confirm({templateName: 'danger'})

stguitar commented 8 years ago

I agree with you about locking into bootstrap - I only made that assumption since this plugin has a dependency on angular bootstrap.

Why couldn't my change simply be adjusted to accept the full list of classes as you specified, wouldn't that actually work for my use case?

I am guessing with your suggestion of named templates, in the angular-confirm code you would define several different templates, and the client code would just supply which one to use? As long as I am understanding, wouldn't this be locked into bootstrap as well? Or perhaps you were expecting that folks would end up customizing the templates to achieve what they want?

I am happy to work on this, once we have the best solution if that helps.

jameskleeh commented 8 years ago

The full list of classes would work for your use case, however I don't want to add a ton of options. Having a singular feature that covers a broader use case is better.

So in the $confirmModalDefaults there would be an additionalTemplates that would be an object where the key is the name and the value would be an object with either a template key or a templateUrl key.

$confirmModalDefaults: {
    additionalTemplates: {
        "warning": {templateUrl: '/../../abc.html'},
        "danger": {template: 'some template string'}
    }
}    

Then $confirm({templateName: "danger"}) would supply the modal with the template for that key. confirm-template-name="danger"

That way you can supply multiple templates for different situations and easily reference them. What do you think?

stguitar commented 8 years ago

This is more flexible, it does leave a bit more up to the client than seems needed (at least for me), but would be easier to support. I also understand wanting to control the number of options, but you are still gaining an option here. This also makes no assumptions about the UI elements needed or being used.

This setup would work for us, we are already defining our own custom template anyway simply to adjust the presentation aspects of the dialog. We would just have to have 2 or 3 templates now - not a big deal.

I am also guessing you were NOT looking for angular-confirm to provide a set of default "additionalTemplates", which keeps you more agnostic as well.

stguitar commented 8 years ago

OK. If you are ok with it, I can proceed to make this work as you described. Just wanted to know we are on the same page before I start that process.

jameskleeh commented 8 years ago

@stguitar Yes that sounds good to me. The module should not provide additional templates, it should be initialized to an empty object.

stguitar commented 8 years ago

Cool. I will probably be able to start on that tonight, so hopefully will have something soon.

stguitar commented 8 years ago

@Schlogen my pull request should be updated now with the proposed changes and test cases. let me know if i need to make any adjustments.

jameskleeh commented 8 years ago

Thanks again @stguitar !

stguitar commented 8 years ago

@Schlogen No prob, happy to help.