Atom-Learning/components

Feature request: <Input type=password/> hide/show value

Closed this issue · 7 comments

hollg commented

<Input type=password/> hide/show value

Description(*)

Our PasswordField component has some logic to show or hide the entered password when the user clicks an eye icon. I think we should move this logic down to the underlying Input component so that we get this behaviour whenever type = "password".

This will allow us to compose other, more specific types of password field and get consistent show/hide behaviour for free.

For example, the CreatePasswordField would get it for free.

is this adding both the function of switching the type back and forth or is it also moving the icon to be part of the input?

We might need to think about our long-term strategy with this, type="number" works well with this convention as we're mocking the behaviour to improve accessibility/usability. type="password" brings a few more assumptions and may not be a one solution fits all scenario.

I can definitely see the benefit of moving that logic out of PasswordField but maybe we need to create a PasswordInput component rather than hooking into type="password".

hollg commented

is this adding both the function of switching the type back and forth or is it also moving the icon to be part of the input?

I was thinking both

We might need to think about our long-term strategy with this, type="number" works well with this convention as we're mocking the behaviour to improve accessibility/usability. type="password" brings a few more assumptions and may not be a one solution fits all scenario.

Interesting — do you have any specific cases in mind where we'd want type='password' without this functionality?

is this adding both the function of switching the type back and forth or is it also moving the icon to be part of the input?

I was thinking both

It might be a bit stretched but adding more buttons and icons to the existing than we have makes it an even more composite component. It is not bad but it pushes the input away from what we thought our primitives would be. not the end of the world to be honest.

I would think Tom's suggestion for a PasswordInput sounds like a good compromise

Interesting — do you have any specific cases in mind where we'd want type='password' without this functionality?

That's an assumption I'd rather not make, type="number" fixes things, there should be no number field that doesn't conform to this new behaviour,type="password" is additive and there may be scenarios where we just need a standard password input.

hollg commented

You two have convinced me :D

So the approach I will take is:

  • Create a PasswordInput with the eye icon and toggling behaviour
  • Update PasswordField to use PasswordInput
  • (Later, in the main app) Update CreatePasswordField to use PasswordInput

@hollg Noice 👌