Skip to content
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(Password): Wrong icon for show/hide password toggle #15825

Closed

Conversation

SoyDiego
Copy link
Contributor

@SoyDiego SoyDiego commented Jun 11, 2024

Fix #15826

⚠️ EDIT

You can read all my research @cetincakiroglu and decide what is the best option to follow because now I don't know what is the correct way.

This topic is a little bit confused, because:

If you see Facebook you can see the solution like this PR:

image

If you check Angular Material Docs too:

image

But If you check Material UI Core, the opposite:

image

And if youread the opinion of ChatGPT you can see this explanation:

When you are implementing a toggle to show or hide the password in an input, the icon should reflect the action that will be performed when clicking on it. Here is the guide for the use of the icons:

    Eye Icon:
        Usage: When the password is currently hidden.
        Purpose: Indicates that clicking will show the password.

    Eye Slashed Icon:
        Usage: When the password is currently visible.
        Purpose: Indicates that clicking will hide the password.

Problem

image

Solution

image

Copy link

vercel bot commented Jun 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
primeng ⬜️ Ignored (Inspect) Visit Preview Jun 11, 2024 6:46pm

@rosenthalj
Copy link
Contributor

@SoyDiego

Your changes have resulted in a mismatch with the show/hide icon templates. I tried to create a pull request to your branch, but failed.

Your PR should be updated with the following changes:

GitHub_Desktop

Note: Your PR fixes the PrimeNG demo, but would break code that were using the show and/or hide icon templates

@SoyDiego
Copy link
Contributor Author

@SoyDiego

Your changes have resulted in a mismatch with the show/hide icon templates. I tried to create a pull request to your branch, but failed.

Your PR should be updated with the following changes:

GitHub_Desktop

Note: Your PR fixes the PrimeNG demo, but would break code that were using the show and/or hide icon templates

Thanks for double check. I will add your code too.
I did fast, thanks again

@SoyDiego
Copy link
Contributor Author

@SoyDiego

Your changes have resulted in a mismatch with the show/hide icon templates. I tried to create a pull request to your branch, but failed.

Your PR should be updated with the following changes:

GitHub_Desktop

Note: Your PR fixes the PrimeNG demo, but would break code that were using the show and/or hide icon templates

Now I did the modifications. Could you check it again, please?

Thanks!

@SoyDiego SoyDiego changed the title Password: The icon to show/hide password is showing the opposite fix(Password): The icon to show/hide password is showing the opposite Jun 11, 2024
Copy link
Contributor

@rosenthalj rosenthalj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the most recent changes, the show/hide icon templates are now consistent with the Eye/EyeSlash icons

@SoyDiego
Copy link
Contributor Author

code show/hide icon templates are now consistent with the Eye/EyeSlash icons

Thanks again, also I edit the description of the PR. I don't know why was giving me 207 files changed for the formatting and right now, anyone. Now I removed the previous text and only I left the important.
Thanks for review :)

@SoyDiego SoyDiego changed the title fix(Password): The icon to show/hide password is showing the opposite fix(Password): Wrong icon for show/hide password toggle Jun 11, 2024
@SoyDiego
Copy link
Contributor Author

I updated my description of this PR after a lot of research.
I think PrimeNG Team should be decide what is the correct way because there are differents opinions

@SoyDiego SoyDiego requested a review from rosenthalj June 12, 2024 19:16
@rosenthalj
Copy link
Contributor

  • I have no idea what the correct solution is
  • I hate to suggested this but a flag could be added as an input to this component to define the behavior
  • A tooltip could be added to the component to describe the "current" state and could also inform the user that the icon can be clicked to change the state

FYI: I don't currently use this component

@cetincakiroglu cetincakiroglu added Status: Pending Review Issue or pull request is being reviewed by Core Team Status: Discussion Issue or pull request needs to be discussed by Core Team labels Jun 13, 2024
@sandrotonon
Copy link
Contributor

sandrotonon commented Jun 14, 2024

IMHO the current behaviour is correct. A button always should indicate what the status (in this case the visibility) WILL BE after clicking, or what its going to do, not what its current status is.

@cetincakiroglu
Copy link
Contributor

cetincakiroglu commented Jul 11, 2024

This one is a design choice I've asked our designer, and he said it depends on the company. In our choice, it refers to the state to be shown next when it is clicked.

@cetincakiroglu cetincakiroglu removed Status: Pending Review Issue or pull request is being reviewed by Core Team Status: Discussion Issue or pull request needs to be discussed by Core Team labels Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Password: Wrong icon for show/hide password toggle
4 participants