Fix CSS class names.

[mvasilkov]

Add .persona-button by default (if none provided). Add color param to browserid_logout, same as _login.

[Osmose]

Thanks for the PR!

Re: Adding .persona-button by default, I don't think I want to accept this change, because it reduces us down to two classes. Our current three classes:

1. browserid-button - This class is used by the JS to hook in default login button behavior. It's optional because users might want a specific button to have different behavior but still use our helper.
2. persona-button - Used for common styling between all the colors of Persona buttons. This is distinct from browserid-button because it's styled like Persona, but it may not necessarily use our default behavior hooked by the browserid-button class.
3. blue, orange, dark - Used to apply colors.

If we remove one, one of those classes of buttons will have to be merged with another, which I don't think we should do. A button should be able to be styled like a Persona button, but not use the default JS behavior, and vice versa. And the color/common styles should be split, otherwise we'd either duplicate code or have to add a pre-processor like Less or Stylus.

Re: Color param to browserid_logout, I'm less strongly against this, but still not sure we should include it, because the logout process doesn't really have anything to do with Persona generally, and I don't want to encourage the idea that you're logging out of Persona like it's an account in itself as opposed to a login system that you sign in with. Have you ever seen a "sign out with Facebook" button?

I summon @willkg to double-check my thinking!

[mvasilkov]

1) Nope, it doesn't. The big idea is as follows:

• .browserid-{login,logout} should be always added in template helpers, because it's a template helper for making a button. (I didn't change it.)
• .persona-button is added in two cases:
  • when user provided color, which means he intends to use the built in CSS classes, or
  • when user didn't provide no custom class, which means he has no styling for it whatsoever. To me, it looks like a good default, especially since the documentation specifically instructs user to include the CSS file, which is then completely ignored otherwise. If provided, custom CSS class replaces .persona-button.

I do follow the logic in what you're saying, except for the part about removing things: that's clearly not what I did. Please see the patch.

2) Having two complementary functions with arbitrarily omitted kwargs is just... Wrong? Let's say I make an orange login button:

{% browserid_login color='orange' %}

Now I want a fitting logout button:

{% browserid_logout color='orange' %}
=> BadlyColoredLogoutButtonException: only the blue version of a logout button is supported

Now that's unexpected.

Just my $0.05.

[Osmose]

.browserid-{login,logout} should be always added in template helpers, because it's a template helper for making a button. (I didn't change it.)

Huh, I completely overlooked line 86 (it is sneaky and has no colors). Sorry!

I'm not sure how I feel about the existing code auto-adding that in, then, because it makes it so that you can't use the helper with the default JS to make a button that doesn't use the default login behavior, but that also sounds like trying to make the helper too flexible. Hmmm...

Either way, you have convinced me of the value of that part, and I'll review the code bits later. :D

=> BadlyColoredLogoutButtonException: only the blue version of a logout button is supported

Never before have I wanted an exception so badly.

Having said that, I still think Persona button styling is still an important visual distinction, and I don't want to encourage sites to use the styling for logout buttons when it doesn't launch a Persona box. It does lead to an asymmetric API, but that's not a big deal IMO.

[willkg]

I'm not convinced that symmetric signatures between login and logout is important here, either. I would not have had that expectation. I'd be curious how prevalent that expectation is.

Beyond that, I sycophantically agree with everything @Osmose says.

[Osmose]

Code looks good, except we need some tests to go along with the change (and some changes to the existing tests to fix the test failures). If you let me know, I can see about getting to adding them myself later.

If you add tests and remove the color argument to the logout helper, I think we can add this in. Nice job!

[mvasilkov]

I updated the patch: – left the _logout signature alone (ah...) – fixed and added tests Please review.

[Osmose]

That test failure is wonky. I'm tempted to just disable that test on those versions.

EDIT: Lies and slander, it makes sense, because the new default value is persona-button, so browserid-login will get appended to it. The rest of the test failures are from persona-button still being added to browserid_logout, so I will be fixing those and merging shortly.

[Osmose]

Merged in b83f5d8. Thanks for the commit!

Also, I forgot to ask to add yourself to AUTHORS.rst, so I added your Github name there myself. Feel free to submit a PR with your full name there if you want. :D

[mvasilkov]

Ow, I just fixed the tests and made changes and stuff.

Thanks ^^