-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: 6555 Missing PT Option for IconField in Password #6560
Fix: 6555 Missing PT Option for IconField in Password #6560
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
@@ -85,6 +85,7 @@ export const PasswordBase = ComponentBase.extend({ | |||
header: null, | |||
content: null, | |||
footer: null, | |||
iconField: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this we aren't adding a new property just a new passthrough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this it wont work 'React does not recognize the iconField
prop on a DOM'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just updated your PR can you do a git pull and test again with my changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
components/lib/password/Password.js
Outdated
@@ -481,8 +481,18 @@ export const Password = React.memo( | |||
let input = <InputText {...inputTextProps} />; | |||
|
|||
if (icon) { | |||
const iconField = props.iconField; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the iconField
part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I thought that in the future it would be easier to access the needed field value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can already change the icons with props.hideIcon
etc.
if (unmaskedState) {
icon = props.hideIcon || <EyeSlashIcon {...hideIconProps} />;
} else {
icon = props.showIcon || <EyeIcon {...showIconProps} />;
}
Looks good to me now what do u people think @KirilCycle @melloware |
I think these latest changes are good @nitrogenous ! |
Defect Fixes
Fix #6555
IconField can be modified from Password component