-
Notifications
You must be signed in to change notification settings - Fork 227
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
bug/Disable the submit button when the variable name field is empty - #576 #578
Conversation
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.
Thanks for the PR @yeokyeong-yanolja !
I have added a small feature to also disable the submit button when no variant is selected.
Closes #567 |
@all-contributors please add @yeokyeong-yanolja for code |
I've put up a pull request to add @yeokyeong-yanolja! 🎉 |
https://github.com/all-contributors please add @yeokyeong-yanolja for bug |
@@ -65,12 +76,13 @@ const NewVariantModal: React.FC<Props> = ({ | |||
<Text>Enter a unique name for the new variant:</Text> | |||
<Input | |||
addonBefore={variantPlaceHolder} | |||
onChange={(e) => setNewVariantName(e.target.value)} | |||
onChange={handleInputChange} // Modify this line |
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.
this comment might be not needed @yeokyeong-yanolja
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 have removed this in a later commit. Thanks for noticing :)
return ( | ||
<Modal | ||
title="Create a New Variant" | ||
open={isModalOpen} | ||
onOk={() => { | ||
setIsModalOpen(false) | ||
addTab() | ||
if (isInputValid) { // Check if the input is valid |
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.
please prefer the early return pattern. this one is okay because no other logics are following and the condition is positive (no negate operation).
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.
@seungduk-yanolja thank you for double checking! Will keep it in mind.
const handleTemplateVariantChange = (value: string) => { | ||
let newValue = value.includes(".") ? value.split(".")[0] : value | ||
setTemplateVariantName(value) | ||
setVariantPlaceHolder(`${newValue}`) | ||
} | ||
|
||
const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => { |
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.
this method name is too general. what input is this?
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.
In this context, input
refers to the name of a variant. will make this clearer in the function name.
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 may create an issue for it
Screen.Recording.2023-09-11.at.5.30.29.PM.mov