search

bluerail / twitter-bootstrap-rails-confirm

Confirm dialogs using Twitter Bootstrap
https://bluerail.nl
MIT License
85 stars 34 forks source link

click handler returns false #6

Closed taavo closed 11 years ago

taavo commented 11 years ago

If you have a link with remote: true, currently confirming it with twitter-bootstrap-rails-confirm causes a "#" to be added to the location. This is because the ".proceed" click handler returns true, allowing the default "a" click handler to fire, so the href "#" gets visited. Nothing breaks if we return false, however, because that's what the callback() method is for.

I've tested this on one of my apps, and it appears to work fine for both remote: true and remote: false.

rvanlieshout commented 11 years ago

Awesome! Thank you for your feedback.

Could you rebase this since i've just merged your previous pull request. Otherwise I'll just copy your change when I have some time to give it a quick run.

taavo commented 11 years ago

Rebased. Thanks!

jonathansimmons commented 11 years ago

I manually made this change to my setup and this did not fix the issue. Any ideas?

taavo commented 11 years ago

Man does this project need a test suite. I could've sworn it was working out here. I'll look into it when I get a chance...

rvanlieshout commented 11 years ago

Exactly. It's still on my TODO-list with the one you suggested in pull request 4

taavo commented 11 years ago

No ideas, @jonathansimmons. I just retested it in one of my apps in the latest Safari, Chrome and Firefox and it really seems to be working. Didn't test IE because I didn't feel like warming up my VM, and I've never had problems with this behavior on IE (...or any browser, really). We'll know more when there's a test suite.

@rvanlieshout, I was hoping to surprise you with a konacha test suite PR, but all of a sudden I'm swamped. Probably won't crawl out from under it for another 2+ weeks.

rvanlieshout commented 11 years ago

@taavo, feeling better already?

Has anybody got the time to test this in IE yet?

taavo commented 11 years ago

Yeah, it's still working out here. Just tested IE8, and it works. I've had this branch running on a pretty active staging server since I issued the pull request, and haven't heard of any problems. Is it working in your tests, @rvanlieshout?

(And, yeah, I'm around. But I may not have the time to set up a test suite for you guys for a while.)

rvanlieshout commented 11 years ago

Then let's get this merged. A new branch is created with a first attempt to get this gem tested using Jasmine. I've tried konacha, but it seems to force me to either create a separate testing app or include Rails in this gem.