-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: DSW-1597: Update input version and update form integrations #110
Conversation
|
||
const handleEmailChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
setEmail(event.target.value); | ||
const handleEmailInput = (event: InputEvent) => { |
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'm not sure if this would be clearer or not (or even work like I think it may), but seeing the repetition made me think of doing something like this:
const handleInput = (handlerFn) => {
return handlerFn((event.target as HTMLInputElement).value);
}
const handleEmailInput = handleInput(setEmail);
const handleUrlInput = handleInput(setUrl);
const handleTelInput = handleInput(setTel);
const handlePasswordInput = handleInput(setPassword);
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.
maybe in a future PR :)
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.
Looks good – one small suggestion that may or not be a good idea, otherwise LGTM
Update the test form to use the newly styled pie-input component and uses the input for all fields in the integration form