search

mozilla / django-browserid

Django application for adding BrowserID support.
Mozilla Public License 2.0
179 stars 80 forks source link

Fix Template tag browserid_logout #269

Closed seocam closed 10 years ago

seocam commented 10 years ago

Fixes #268.

Osmose commented 10 years ago

Not bad, but why only fix the logout button? The login button suffers from the same issue.

But then, once you make that fix, you'll run into another issue; the admin integration will break.

We used to actually add these classes by default, but then I removed them. See, what happened was that, before api.js and browserid.js were separate files, they were a single file; you couldn't use the JavaScript API without also adding event handlers for all browserid-login and browserid-logout buttons.

This was a problem for the admin integration, as we need some custom logic there for showing an error when login fails. If these helpers force you to have those classes, and the JS files are both on the page, without some funky event hacking your custom JS will be overridden by the event handlers from browserid.js.

Luckily, now that they're separate files, if you make the login and logout add their default classes and remove browserid.js from the admin login template, everything works out fine. Do that, and I think this PR will be ready for merging. :D

(Thanks for submitting what you have thusfar, by the way!)

seocam commented 10 years ago

@Osmose for the login we have two default classes browserid-login and persona-button. Should we make sure that we always have both classes there?

It seems weird to me because I don't actually want to use the persona-button style so if we keep it I'll have to override the it in my CSS.

For now I'll only enforce browserid-login.

Osmose commented 10 years ago

@Osmose for the login we have two default classes browserid-login and persona-button. Should we make sure that we always have both classes there?

How about we only add persona-button if the color argument is passed? Seems like a decent compromise.

seocam commented 10 years ago

Ok. I'm having some issues with the admin here.

You were right about the admin been broken. When I try to login it redirects me back to the login page. If I try to remove the browserid.js file the login dialog doesn't even show up. I'm starting to investigate that.

Osmose commented 10 years ago

Looks good, r+! Thanks for the fix! :D