Fix bug 1335331 - Enable users to change their email address #793
Conversation
This commit removes the fancy javascript that allowed to save user profile without reloading the page. It was a big source of errors and complexity. The settings page is now much simpler, the custom home page being the only exception. This fixes the CSRF error that was happening as well when saving data by clicking the Save button.
| {{ profile_form.email }} | ||
| {{ profile_form.email.errors }} | ||
| <span id="email-warning"> | ||
| Caution: Changing your email address will cause a logout.<br / /> |
| #username .container input:focus + .submit:hover { | ||
| color: #7BC876; | ||
| #id_email:focus + span{ | ||
| display: block; |
There was a problem hiding this comment.
contributor.css is shared between the /settings and /profile page. All the newly introduced CSS rules should be moved to the settings.css file, because they don't have any impact on the /profile page.
| #username .container input { | ||
| background: transparent; | ||
| border-bottom: 1px dashed transparent; | ||
| #profile-form .field input { |
There was a problem hiding this comment.
Separate CSS rules with a blank line.
| #email-warning { | ||
| display: none; | ||
| margin-top: 5px; | ||
| color: red; |
There was a problem hiding this comment.
Let's use Pontoon red here - #F36.
And let's apply the same style (margin + color) to the .errorlist (e.g. "Email address must be unique.") and also set list-style: none;.
| messages.success(request, 'Settings saved.') | ||
| return redirect(request.POST.get('return_url', '/')) | ||
|
|
||
| if user.email != request.user.email: |
There was a problem hiding this comment.
Do we need this check? UserProfileForm should be invalid if the condition is true, so we never reach this code.
There was a problem hiding this comment.
If the user didn't change their email, then there's no reason to disconnect them. Unless I'm missing something, this line is needed for that reason, and it does get called. It's possible for the user to edit their name and not their email, in which case the form is valid but the email did not change.
There was a problem hiding this comment.
You're correct. Sorry for the noise.
It was bad that the warning for a changed email would only appear on focus on the input. First it made it difficult to click the save button, as the layout was changed and thus the button moved. Second, it would show up even if the email was not changed. This commit also moves CSS to the more specific settings.css file, and addresses a few review comments.
|
@mathjazz I've added a new commit that does more than just addressing your comments, mind giving it a look? Rational for the change is in the commit message. |
| #email-warning { | ||
| display: none; | ||
| } | ||
| #email-warning, |
| else if (newEmail !== originalEmail && !warningContent.is(':visible')) { | ||
| warningContent.show(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I'm not a big fan of this. It doesn't work if you paste the email address using the mouse and the context menu for example. It also adds a lot of code.
Unrelatedly, there's a usability issue with this text - it gets overflown by the autocomplete box. We could move it to the right of the field, and possibly make it fit into two lines.
There was a problem hiding this comment.
I agree about the usability issue, and I'm going to fix it, but I don't understand your point about "a lot of code". That's only 10 lines, and I don't think it can be done with much less. The CSS-only solution had a different bunch of usability issues. Do you think we should do without that feature entirely instead?
There was a problem hiding this comment.
I'd definitely keep the message, because changing the email address leads to an unexpected action.
It's a lot of code relatively speaking - compared to the CSS solution. What are the usability issues there?
We could also make the text always visible (althout not red in this case) or make it pop up on form save if the email address changes.
There was a problem hiding this comment.
Quoting my commit message:
It was bad that the warning for a changed email would only appear on focus on the input. First it made it difficult to click the save button, as the layout was changed and thus the button moved. Second, it would show up even if the email was not changed.
I was going to consider going for a tooltip to the right of the input box instead of a red text below. What do you think?
There was a problem hiding this comment.
You could circumvent the layout change by toggling the visibility property instead of display. But that would still make the UI "blink".
Tooltip sounds good. Do you want to do something fancy? Helper text would also work:
|
New proposal:
|
| #profile-form .help { | ||
| color: #FED271; | ||
| font-style: italic; | ||
| font-weight: bold; |
There was a problem hiding this comment.
Now that the text is always visible, I find the styling a little too intrusive.
I'd remove bold (bold+italic at the same time is rarely a good idea) and change color to #888888. Please also add margin-top: 5px;.
|
Success! Thanks a lot again to @karabellyj for your contributions! 😃 |
Taking over from #631. This is exactly the same content, plus some changes I've made on top, and rebased against latest master.
Lots of credits go to @karabellyj! Thanks again for your work!
@mathjazz, mind making sure I haven't done anything stupid?