-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix buttons problems in 'Change Email' section. #5219
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
Fix buttons problems in 'Change Email' section. #5219
Conversation
@humitos |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This is still valid bot. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Still valid bot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
I left some small styling comments.
{% for emailaddress in user.emailaddress_set.all %} | ||
<div class="ctrlHolder"> | ||
<label for="email_radio_{{forloop.counter}}" class="{% if emailaddress.primary %}primary_email{%endif%}"> | ||
<input id="email_radio_{{forloop.counter}}" type="radio" name="email" {% if emailaddress.primary %}checked="checked"{%endif %} value="{{emailaddress.email}}"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to use spaces around {{
and {%
. For example, {{ forloop.counter }}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
I have pushed the changes.
<button class="primaryAction" type="submit" name="action_remove" >{% trans 'Remove' %}</button> | ||
</div> | ||
{% if user.emailaddress_set.all %} | ||
<p>{% trans 'The following email addresses are associated with your account:' %}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we have been using "
for these blocks.
var primaryButton = document.querySelector("button[name=action_primary]"); | ||
var sendVerificationButton = document.querySelector("button[name=action_send"); | ||
|
||
{% if user.emailaddress_set.all %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use .exists
here instead of .all
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
I have pushed the changes.
We could, but I think this is not really important in this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this locally without issue and I think it's safe to merge. With that said, I think things like this get a lot easier once we have a better UI framework in place.
Are there any plans to work on the UI part of RTD? |
Yes there are. This has been something I've been pushing on for a while but it's actually a pretty huge task. |
@davidfischer |
I'm merging the PR now since it's tested and we agree on it. |
Problem it solves:
Resend verification link
button and after it has been verified, hideResend Verification link
button and showMake primary
button.extra_body
. It should befooterjs
.Demo:
Currently, I have implemented this on client side, this should also be from server side. I was able to send 3-4 email verification email for a verified email.