Show password field mask while editing service settings
What does this MR do?
While editing service settings from Project > Settings > Integrations, password field label says Change password while the password input field itself remains blank. This MR adds mask to the field while editing the form.
Screenshots
Inactive Service (without any configuration)
Active service (with configuration present)
Active service (when password change is attempted)
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary [ ] Documentation created/updated[ ] API support added- Tests
[ ] Added for this feature/bug-
All builds are passing
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Closes #31510 (closed)
Merge request reports
Activity
Hi @tauriedavis can you review and approve how the mask appears after this MR?
/cc @victorwu
assigned to @victorwu
assigned to @tauriedavis
-
@tauriedavis Please note that since integration settings form is shared for all services, this change will apply to every service where we ask for password.
marked the checklist item Squashed related commits together as completed
marked the checklist item Conform by the style guides as completed
marked the checklist item Conform by the merge request performance guides as completed
added 2 commits
marked the checklist item Changelog entry added, if necessary as completed
Sorry @kushalpandya, I'm kind of confused. I looked at master and this branch, but I can't tell what is different. The password field is masked on master already.
-
@tauriedavis I meant the mask that we show when user has already set password as a part of service configuration but sees empty textbox instead, here's how it looks in
masterright now.
Notice how field label says "Change password", and by seeing an empty input next to it, user always assumes that he/she needs to provide password everytime he wishes to change configuration other than password itself. I hope this clears the confusion.
-
@tauriedavis Having seen similar UI at many different apps, I feel a more appropriate label would be to have
Passwordwhile mask saying it[Change Password]. Something like this;
-
@kushalpandya I was looking at how iOS does their account management. If you're signed into an app, you just have the Password field like you mentioned. I think that makes a lot more sense. The field is masked, though, with the password. So you can change it if you want, but it's clear there is a password entered. Is there a security reason we don't do this same thing?
-
Is there a security reason we don't do this same thing?
This is actually a debatable UX topic, as I haven't seen any webapp where editing form shows my existing password, even as masked (other than browser doing it for me with Auto-fill enabled).
Here are some resources around the topic that I referred to;
Showing Passwords on Log-In Screens by Luke Wroblewski
Better Password Masking For Sign-Up Forms on Smashing Magazine
-
I like the suggestions here: https://ux.stackexchange.com/questions/45150/how-should-i-show-a-password-exists-on-an-edit-password-form
I think the second option is a good one too, show the time it was lasted updated and with a link to change it. That might be outside of the scope. But keeping the input blank with the label "Enter new password" is a good alternative. What do you think?
-
@tauriedavis Yes, that works! Here's how it looks with the change.
assigned to @kushalpandya
added 167 commits
-
60aa12d5...01a7f333 - 161 commits from branch
master - 5f44acf3 - Add pasword field mask while editing service settings
- 0aafe23b - SCSSLint: Use double colon fir pseudo elements
- 4365c747 - Add changelog entry
- 02ea20bd - HAMLLint: Remove unnecessary whitespace
- 552801b7 - Change password field label during edit
- 9eb049f6 - Update changelog description
Toggle commit list-
60aa12d5...01a7f333 - 161 commits from branch
-
@iamphill Can you review and merge this please? UX change in this MR is already approved, screenshots in description reflect the same.
assigned to @iamphill
assigned to @kushalpandya
-
@iamphill Updated tests. Back to you.
assigned to @iamphill
mentioned in commit 6233e56e
