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

Update _RenderFormInput.tsx featue Ehancement #67

Closed
wants to merge 6 commits into from

Conversation

Ktbch
Copy link
Contributor

@Ktbch Ktbch commented May 7, 2024

implementation of Show password functionality
on the auth page

Pull Request type

Please check the type of change your PR introduces:

  • Feature

What is the current behavior?

the current behavoir of the password field of the input field does not have the functionality that enables the user to see the password in plain text

i implemented the function that enables the user to show password in plain text
I

What is the new behavior?

Does this introduce a breaking change?

  • No

Other information

implementation of Show password functionality
on the auth page
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

case "url":
case "color":
return <FormInput type={type} {...formProps} />;
case "password":
return (
Copy link
Contributor

@thrownullexception thrownullexception May 7, 2024

Choose a reason for hiding this comment

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

Can we move this to a new component, FormPasswordInput

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay sir

...formProps.input,
onChange: (val) => {
formProps.input.onChange(val);
if (!formProps.input.value) isPasswordReveal.off();
Copy link
Contributor

Choose a reason for hiding this comment

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

The switch should be intentional not hooked to the onChange handler

action: () => {
isPasswordReveal.toggle();
},
label: msg`show Password`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label: msg`show Password`,
label: msg`Show Password`,

formProps.input.value
? [
{
systemIcon: "Eye",
Copy link
Contributor

@thrownullexception thrownullexception May 7, 2024

Choose a reason for hiding this comment

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

Add this

  EyeOff: `<path d="M17.94 17.94A9.995 9.995 0 0112.009 20H12c-7 0-11-8-11-8a18.605 18.605 0 015.017-5.908l.043-.032M9.9 4.24A8.865 8.865 0 0111.979 4h.023-.001c7 0 11 8 11 8a18.62 18.62 0 01-2.182 3.217l.022-.027m-6.721-1.07a3 3 0 11-4.242-4.238l.002-.002M1 1l22 22" />`,

to src/shared/constants/Icons.tsx so that we have a EyeOff icon

Copy link
Contributor

@thrownullexception thrownullexception left a comment

Choose a reason for hiding this comment

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

Would also be nice, if we could have some unit tests

type={isPasswordReveal.isOn ? "text" : "password"}
{...formProps}
rightActions={
formProps.input.value
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above the logic of the rightAction always be present and should be based on the isPasswordReveal.isOn so we should be having something like

rightActions: [showPassword.isOn ? {
systemIcon: 'EyeOff',
 action: showPassword.toggle,
  label: msg`Hide Password`,
}: {
systemIcon: 'Eye',
 action: showPassword.toggle,
  label: msg`Show Password`,
}]

Copy link

@Betheldev123 Betheldev123 left a comment

Choose a reason for hiding this comment

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

update of showPassword feature

case "url":
case "color":
return <FormInput type={type} {...formProps} />;
case "number":
return <FormNumberInput {...formProps} />;

case "password":
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move all the logic here to the new password component
Like here it should just be

   return <FormPasswordInput {...formProps} />;

everything else should be in the component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got It

import { Input } from "./Styles";

interface IFormInput extends ISharedFormInput {
type?: "password" | "text";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed, you can just pass ISharedFormInput as the component props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!!

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.

4 participants