Skip to content
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

feat: wrapper option #233

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

HollyPony
Copy link

Add a simple wrapper option to encapsulate the template in a div.

I find it useful to manage dynamic modal based on routes (like my own use case), and not have to manage a third part element with appendElement option.

I'm open to discussions (since it's my first contact with you 🤗)

@dwmkerr
Copy link
Owner

dwmkerr commented Feb 6, 2018

Looks good! Seems like a useful option. It would be good to include a couple of tests, maybe one to assert that when wrapper is true, the element is wrapped, and another to test that if wrapperClass is provided, it is set on the element, WDYT?

@HollyPony
Copy link
Author

Yeah that's a great idea, I've made a quick fix for my own purposes. And, finally, I have rewrite the whole service to integrate the component feature. It's still in progress work (you can check it here : https://github.com/HollyPony/angular-modal-service) .

I'll look for unit test and a document.createElement approach.

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.

2 participants