Skip to content

Feature/Support for additional custom classes #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 22, 2018
Merged

Feature/Support for additional custom classes #26

merged 3 commits into from
Mar 22, 2018

Conversation

elibolonur
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

  • One wouldn't be dependent on the data-attribute custom class in dialog-window.
  • It would also be easier to add more custom classes in the future (no need to define constants). Defining constants is however much more readable and easier to communicate to users of the plugin!

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
Copy link
Contributor Author

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
Copy link
Owner

@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
Copy link
Contributor Author

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

@hjortzen
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Owner

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

@Godofbrowser Godofbrowser changed the base branch from master to dev March 22, 2018 21:22
@Godofbrowser Godofbrowser merged commit 23df0c3 into Godofbrowser:dev Mar 22, 2018
@Godofbrowser
Copy link
Owner

@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
Copy link
Contributor Author

Great, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants