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

click handler returns false #6

Merged
merged 1 commit into from
Apr 24, 2013

Conversation

taavo
Copy link
Contributor

@taavo taavo commented Mar 10, 2013

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

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

taavo commented Mar 10, 2013

Rebased. Thanks!

@jonathansimmons
Copy link

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

@taavo
Copy link
Contributor Author

taavo commented Mar 26, 2013

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

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

@taavo
Copy link
Contributor Author

taavo commented Mar 27, 2013

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

@taavo, feeling better already?

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

@taavo
Copy link
Contributor Author

taavo commented Apr 23, 2013

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 added a commit that referenced this pull request Apr 24, 2013
@rvanlieshout rvanlieshout merged commit 6509be6 into bluerail:master Apr 24, 2013
@rvanlieshout
Copy link
Member

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.

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.

None yet

3 participants