Godofbrowser / vuejs-dialog

A lightweight, promise based alert, prompt and confirm dialog
351 stars 111 forks source link

Feature/Support for additional custom classes #26

Closed elibolonur closed 6 years ago

elibolonur commented 6 years ago

Options now takes in a property named customClass object to be able to inject custom class names in to the elements.

Example:

const options = {
  html: false,
  loader: false,
  customClass: {
    mainContent: "",
    body: "bg-black",
    footer: "",
    ok: "green bg-white",
    cancel: "red"
  }
}

customClass properties are optional and users can prefer to inject a single or all classes. Moreover, users can inject multiple classes to a single element.

hjortzen commented 6 years ago

Aaah, you beat me! I did a very similar thing in my local branch, almost ready to merge. I'll make a quick review of your PR and see if I can contribute anything to your solution?

hjortzen commented 6 years ago

Nice job! It doesn't clutter the template code either. My solution used a mixin that components imported, with a method to pull a dynamic property from options. The upside to that solution could be

Anyway, the only downside I see to the PR is possibly (note: not a real use case right now) that you wouldn't be able to set a custom class to an element internally within a component, for instance dg-btn-content in ok-btn.vue, since referencing parent data is a bad idea.

I'd like to see a custom class on the h6-elements (title if specified)!

elibolonur commented 6 years ago

Thank you for the review :)

I have completely missed title part, but it would be nice to have that part also. I thought about the referencing parent data and child components as well but focused more on a fast solution. But I (or we) could definitely fix that part also. By the way, you are very welcome to contribute/edit my changes :)

Godofbrowser commented 6 years ago

@hjortzen @elibolonur thanks for the good work. Would you prefer I create a new branch (development branch) such that every pull request is first merged into that branch and modified if need be and merged into master when complete? This pull is nice but I think your(@hjortzen) solution would be easier to maintain. Maybe you can edit @elibolonur 's commit to work like yours?

elibolonur commented 6 years ago

Welcome :) @Godofbrowser dev branch would be nice to have!

hjortzen commented 6 years ago

I'm sorry for being slow to respond, illness within the family :( A dev branch would work very well I think! Especially since I believe that would open up possibilities to fix or touch up PR's (like adding test cases, defining constants that the PR missed or similar).

hjortzen commented 6 years ago

Btw, I recently pushed my semi-done solution into my branch... That said, I don't think my solution is necessarily better. Though the use of mixins is probably a good idea.

Godofbrowser commented 6 years ago

I'm sorry about that @hjortzen , i hope all is well now.

Godofbrowser commented 6 years ago

@elibolonur PR merged. Thanks. I still believe a few improvements can be made to the implementation on the dev branch before finally merging into master.

@hjortzen I'm checking out your implementation. @elibolonur 's done a great job and i'm looking for a way to make the config available to child components as well.

On the other hand, i still think its equally possible to override the dialog's styles without writing new rules. For example: if you already have

.btn-success {
    background-color: green;
}

this can be extended to the dialog's button by doing

.btn-success, .dg-btn--ok {
    background-color: green;
}
elibolonur commented 6 years ago

Great, thanks :)