search

atk4 / ui

Robust and easy to use PHP Framework for Web Apps
https://atk4-ui.readthedocs.io/en/stable/
MIT License
447 stars 106 forks source link

Add reveal eye to password form control #2219

Closed rickyosser closed 2 months ago

rickyosser commented 3 months ago

This is an addition to the Password form-control which adds an eye to toggle password visibility.

The JS-script part maybe should be more ATK4 friendly but that is above my current knowledge, I will learn with help.

rickyosser commented 3 months ago

Hi, I understand the codingstyle errors and stuff but need help to decode the StaticAnalysis errors. 1, Overrride of the init() function, this might not be what I want to do, how would I go about to implement this as an addition/extend which will run automatically if the variable $this->eye is set to true? 2, I don't understand this at all, please enlighten me...

Thanks in advance. Rickard

rickyosser commented 3 months ago

Nice changes! I wondered how to find the element id, now I have an example for future use, thank you!

rickyosser commented 3 months ago

@mvorisek, I copied Password.php to Password2.php and added another test-group for password. You can review both versions side-by-side. If the new version is better I'll replace the original version.

rickyosser commented 3 months ago

@mvorisek, I copied Password.php to Password2.php and added another test-group for password. You can review both versions side-by-side. If the new version is better I'll replace the original version.

Ok, an unintended difference with this new approach is that the whole field is clickable. So, by clicking the field it toggles it, not only the icon.

There is another approach which is to add another button to the input field, there is a variant of this in the decorated input demo.

mvorisek commented 3 months ago

On the right side, there should be transparent button with the reveal icon. And keep the possibility for regular right action/button.

IDK if there is Fomantic-UI style for it, if yes, great, if not, lets add minimal HTML/CSS and see if we can contribute it then into Fomantic-UI repo.

rickyosser commented 3 months ago

On the right side, there should be transparent button with the reveal icon. And keep the possibility for regular right action/button.

IDK if there is Fomantic-UI style for it, if yes, great, if not, lets add minimal HTML/CSS and see if we can contribute it then into Fomantic-UI repo.

Ok, I rewrote parts of it to not use the action-field, it does work when the input field is referenced (as in the code I cecked in) but in the same way as with icon, the whole field just toggles. When I create the button it has an event, as long as all code is in the init() function, when I save the button to a private variable in the class then the events can't be trapped in the recursiveRender() function. Checking with the inspector in FF there is no event associated with the button itself.

Fomantic class for buttons supports basic and tertiary, basic makes a button with outlines and no bg-color, tertiary removes the outline as well.

That's what I found out so far. If there is a way to force the event for just the button then this would work.

I think that the icon version looks nicer though.

rickyosser commented 3 months ago

Ok, now I got it working, the question is how to move the button to the inside of the input-field, if that is necessary.

rickyosser commented 3 months ago

The demo looks like this now: image

mvorisek commented 3 months ago

The last row of examples looks very promising!

Let's minimize the changes to only this version and then let's do the tests.

mvorisek commented 2 months ago

Well done. One more question - you initially introduced revealEyeIcon property. I would support it, the question is, can two icons be put inside the right side of input element by FUI? Imagine someone using the icon for marking required fields etc.

mvorisek commented 2 months ago

And one more question - https://jsfiddle.net/fv2bwyq5/ - In the demo the focus is kept when a div is clicked. I do not fully understand why, but the reveal/unreveal click currently removes focus from the password, but the focus should be kept as in the demo when a div is clicked.

rickyosser commented 2 months ago

Well done. One more question - you initially introduced revealEyeIcon property. I would support it, the question is, can two icons be put inside the right side of input element by FUI? Imagine someone using the icon for marking required fields etc.

Thanks, even though you've made it better... :) Anyway, to answer your question, yes, according to the code, I haven't tried it, but you should be able to add another icon either with BeforeInput or AfterAfterInput and the eye will always be AfterInput. I'll try and see if it works tomorrow with the old code before your changes tonight.

rickyosser commented 2 months ago

And one more question - https://jsfiddle.net/fv2bwyq5/ - In the demo the focus is kept when a div is clicked. I do not fully understand why, but the reveal/unreveal click currently removes focus from the password, but the focus should be kept as in the demo when a div is clicked.

I checked 2 different applications, 1 an AccessPoint looses focus on the password field when the eye is pressed, Acronis on their on-line management platform doesn't. Anwyay, it is an easy fix, I'll commit it now.

mvorisek commented 2 months ago

And one more question - https://jsfiddle.net/fv2bwyq5/ - In the demo the focus is kept when a div is clicked. I do not fully understand why, but the reveal/unreveal click currently removes focus from the password, but the focus should be kept as in the demo when a div is clicked.

I checked 2 different applications, 1 an AccessPoint looses focus on the password field when the eye is pressed, Acronis on their on-line management platform doesn't. Anwyay, it is an easy fix, I'll commit it now.

Based on https://stackoverflow.com/questions/8735764/prevent-firing-focus-event-when-clicking-on-div I meant to use native mousedown as click even can no longer cancel the focus, but I did some testing and your elem.focus() solution seems to have better UX and is more consistent how other elements remove focus. 👍

So only https://github.com/atk4/ui/pull/2219#issuecomment-2341915511 remains and we are ready to go!

rickyosser commented 2 months ago

Well done. One more question - you initially introduced revealEyeIcon property. I would support it, the question is, can two icons be put inside the right side of input element by FUI? Imagine someone using the icon for marking required fields etc.

It was a neat idea, it doesn't work in practice! :) Let's stay with the icon replacement. If the developer wants to add something else he can turn of revealEye or add a label/add action.

mvorisek commented 2 months ago

I love this PR, thank you for this core contribution ❤