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
Add proverb form component #24
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.
Great job!!! Added a couple of comments to make the code a bit more readable in some areas
client/src/components/Modal.js
Outdated
<ModalBootstrap.Header | ||
className="border-bottom-0" | ||
closeButton | ||
></ModalBootstrap.Header> |
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.
></ModalBootstrap.Header> | |
/> |
since this component has no children, you can close it within the same <>
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.
Absolutely.
client/src/pages/home/HomePage.js
Outdated
const [modal, setModal] = useState({ | ||
isOpen: false, | ||
type: "", | ||
id: "", |
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.
do we need the id
field here?
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.
No. For now we dont need it.
client/src/pages/home/HomePage.js
Outdated
|
||
//Modal Handlers | ||
|
||
const handleShow = (type) => async (e) => { |
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.
const handleShow = (type) => async (e) => { | |
const handleShowModal = (type) => async (e) => { |
Let's make this function name even more specific so it is very clear what it is for
client/src/pages/home/HomePage.js
Outdated
<Section | ||
<Modal | ||
isOpen={modal.isOpen} | ||
modalClose={() => setModal({ isOpen: false })} |
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.
modalClose={() => setModal({ isOpen: false })} | |
modalClose={() => setModal({ isOpen: false, type: undefined })} |
I think we should reset the state here as well, so that when we add other modal types (for example Edit), then we wont accidentally open the add one
client/src/pages/proverb/Proverb.js
Outdated
Boolean(data) | ||
); | ||
const result = isArabic(proverb); | ||
isFilled && result ? setDisabled(false) : setDisabled(true); |
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.
Lets extract this into 2 lines so it is more readable:
const shouldBeDisabled = !isFilled || !result;
setDisabled(shouldBeDisabled);
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 like it. Thank you
client/src/pages/proverb/Proverb.js
Outdated
isFilled && result ? setDisabled(false) : setDisabled(true); | ||
}, [formData]); | ||
|
||
const onChange = (e) => { |
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.
const onChange = (e) => { | |
const handleInputChange = (e) => { |
Lets make this function name more explicit as well, to make it even more readable :)
client/src/pages/proverb/Proverb.js
Outdated
type="text" | ||
value={proverb} | ||
name="proverb" | ||
onChange={(e) => onChange(e)} |
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.
onChange={(e) => onChange(e)} | |
onChange={handleInputChange} |
client/src/pages/proverb/Proverb.js
Outdated
type="text" | ||
value={translation} | ||
name="translation" | ||
onChange={(e) => onChange(e)} |
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.
onChange={(e) => onChange(e)} | |
onChange={handleInputChange} |
client/src/pages/proverb/Proverb.js
Outdated
type="text" | ||
value={explanation} | ||
name="explanation" | ||
onChange={(e) => onChange(e)} |
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.
onChange={(e) => onChange(e)} | |
onChange={handleInputChange} |
client/src/pages/proverb/Proverb.js
Outdated
<Button | ||
variant="info" | ||
text="Add" | ||
onClick={(e) => onSubmit(e)} |
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.
onClick={(e) => onSubmit(e)} | |
onClick={onSubmit} |
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.
Yes my fault. We dont need to create a new function.
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.
great Salih! I only added a couple of small comments.
client/src/helpers/functions.js
Outdated
@@ -0,0 +1,5 @@ | |||
export const isArabic = (text) => { | |||
const pattern = /[\u0600-\u06FF]/; | |||
const result = pattern.test(text); |
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.
const result = pattern.test(text); | |
return pattern.test(text); |
this can be simplified as we are not using the result value.
client/src/pages/home/HomePage.js
Outdated
<Section | ||
<Modal | ||
isOpen={modal.isOpen} | ||
modalClose={() => setModal({ isOpen: false, type: undefined })} |
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.
modalClose={() => setModal({ isOpen: false, type: undefined })} | |
modalClose={handleCloseModal} |
you could create a function called handleCloseModal and use it as I wrote above. this could be an implementation:
function handleCloseModal() {
setModal({ isOpen: false, type: undefined })
}
It's good to have named functions, they are easier to read and to debug than anonymous functions. I would advise
using arrow functions when they really improve the readability of the code or when you want to avoid context issues related to using the "this" object.
You can find a lot of interesting information about arrow functions here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions
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.
🎉
Created Proverb Form component
Created Modal reusable component to show proverb add page inside it.
Add some styling