search

plwalters / aurelia-bs-modal

DEPRECATED - Aurelia plugin for bootstrap modal
MIT License
17 stars 16 forks source link

Allowing The Model To Be Set For The Modal #3

Closed lipsitzm closed 9 years ago

lipsitzm commented 9 years ago

I needed the ability to pass values into the view-model of the modal so I exposed the model property for binding.

I definitely didn't read the contribution guidelines before forking and commiting my changes so it doesn't match the expected commit message format. Let me know if that's an issue and I'll re-do it.

plwalters commented 9 years ago

Awesome I will review this in the morning! Thanks!

lipsitzm commented 9 years ago

So my branch has gotten a decent ways behind yours and a few of the changes that I had to do aren't needed anymore since you added back in jQuery (which definitely makes using it easier) so I'm going to delete this branch and re-do it rather than deal with the conflicts from the jquery changes.

Before I do though, I was able to use the modal without binding an object to the model, so what did you mean by making it an optional binding? Think it should just have a default value or is there a more Aurelia-way of doing it that I haven't seen?

plwalters commented 9 years ago

@lipsitzm Just wanted to make sure that it doesn't break if you don't supply a model in cases where the developer doesn't want to pass the model down.

plwalters commented 9 years ago

Sorry for not merging it earlier I needed to get fixes out on the last release and didn't have time to get everything done, thanks for re-submitting. I get it in this time for sure.

lipsitzm commented 9 years ago

Good deal. Definitely makes sense to check for that, I just wasn't sure if there was a keyword that I hadn't seen / made it into the documentation yet to designate it as truly optional...

And no worries at all. I'll get it recreated with a new PR in the next few days.