-
Notifications
You must be signed in to change notification settings - Fork 965
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
Move show password checkbox under password field #8027
Conversation
I could also move "show password" down to be horizontally aligned with "Log in" and "forgot password." I wasn't sure where exactly you wanted it. This does meet the ticket's request of making password one tab away from username. |
Yes, please do that! I think that can be done with the |
Ok, I removed the How does d845979 look? I did the following:
New screenshot below |
Ah, I didn't realize that was the goal. Your suggested diff doesn't quite do it, since But when I wrap the |
@nlhkabu No rush, but just a friendly ping on this one in case it got lost. |
Hi @sanjaysiddhanti thank you for your pull request. I think it's beginning to feel a bit crowded down at the bottom right. Maybe a layout like this would be better: You can use @di I am going to remove myself as a reviewer here (lack of bandwidth) - please merge once @sanjaysiddhanti has addressed my feedback and you are happy to go ahead. Thanks 🙏 |
<div> | ||
<input type="submit" value="{% trans %}Log in{% endtrans %}" class="button button--primary"> | ||
<div class="form-group"> | ||
<div class="split-layout margin-top--large margin-bottom--large"> |
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.
removed split-layout--table
, as it causes elements to be vertically centered in the container, whereas Nicole's screenshot shows "Show password" aligned to the top of the div
Thanks for the feedback! Here is an updated version: The only difference I see is that Nicole's version has double the margin between "Log in" and "Show password." I tried to achieve this with the following, but the margins were getting collapsed with the sibling element:
I also tried wrapping the @di did you have an idea of how to get this extra margin and does it matter to you? |
Since there's now a new feature request regarding a "Keep me signed in" button at #8033, would it be beneficial to switch to a 👁️ icon, as mentioned by @bittner at #6722, to prevent a clutter of text fields under the password field? Any thoughts? @di @sanjaysiddhanti |
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 think this is good as-is!
Nice! This change fully addresses the usability issue that was there before. Adding an 👁️ icon now only will be a cosmetic enhancement. This can certainly be done "any time". 👍 Thanks for this important improvement! 🥇 |
Nice, this change is already rolled out. Is the pipeline that does this public? It doesn't seem to be the GitHub Actions of this repository. 🤔 |
Hi @sanjaysiddhanti! Congrats on landing your first pull request in PyPA projects. Could I ask you to take a look at #5222 and comment there if you can work on it? |
@di
This PR implements #6722.
Screenshot before:
Screenshot after:
Let me know if you'd like any additional changes.
Fixes #6722.