Skip to content

Feature/create movie form#12

Open
Hezanin wants to merge 12 commits intomasterfrom
feature/create-movie-form
Open

Feature/create movie form#12
Hezanin wants to merge 12 commits intomasterfrom
feature/create-movie-form

Conversation

@Hezanin
Copy link
Copy Markdown
Collaborator

@Hezanin Hezanin commented Jul 11, 2024

Add form for creating movies.

This pr adds a way of creating a new movie on the create movie page using Formik for the form submission.
-The form contains some basic input validation to make the fields required, Yup library was used for this purpose.
-This also includes a success message that disappears after 5 seconds which contains the id and the name of the newly created movie. An error message is also displayed on bad creation requests.

Screenshot_127
image
image

Hezanin added 8 commits July 11, 2024 14:16
Add form input fields file that define the required input fields to create a movie;
Add style dir and style file for movie form;
Add custom hook for movie post request
Update movie form component to accept an onSubmit prop and call it when the form is submitted;
Update movie form component to include styling;
Update create movie component to integrate form and form submission;
movieForm component was renamed to createMovieForm;
Remove unused imports;
Update display of form submission feedback on create movie page to separate be components
Update form with addition of input validation using Yup library that requires both movieId and movieName to be non-empty strings;
Add button in movies page for routing to create movie page;
Update display of success message after creating a movie to dissapear after timed period (set to 5 seconds)
const FormField = ({ id, label, formik }) => (
<div className="formGroup">
<label className='formLabel' htmlFor={id}>{label}</label>
<input
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The Field component from formik also handles errors inside. It's a good practice not to reinvent the wheel.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

A bit of a larger commit for this but i think it's an improvement, see 40cbf22 and 846c45f for the field

const { mutate, data, isSuccess, isError, error } = useCreateMovie();

} No newline at end of file
const handleFormSubmit = (values) => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is just a renaming for the mutate function. We could rename it where we destructure it from the useMutation

const { mutate: handleFormSubmit, data, isSuccess, isError, error } = useCreateMovie()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Solved at 6c63366

Copy link
Copy Markdown
Owner

@OctaFloare OctaFloare left a comment

Choose a reason for hiding this comment

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

Really nice work. Good extraction, added validation. Also love the user experience you are always considering, like the DisplaySuccess that shows a feedback to the user and afterwards disappears

@@ -0,0 +1,21 @@
import React from 'react';

const FormField = ({ id, label, formik }) => (
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
const FormField = ({ id, label, formik }) => (
import React from 'react';
const FormField = ({ id, label, formik }) => {
const { handleChange, handleBlur, values, errors, touched } = formik
return (
<div className="formGroup">
<label className='formLabel' htmlFor={id}>{label}</label>
<input
id={id}
name={id}
type="text"
onChange={handleChange}
onBlur={handleBlur}
value={values[id]}
className="input"
/>
{touched[id] && errors[id] ? (
<div className="text-red-500 text-sm mt-1">{errors[id]}</div>
) : null}
</div>
);
};
export default FormField;

@@ -0,0 +1,11 @@
import React from 'react';

const DisplayError = ({ error }) => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should consider returning the jsx directly instead of using curly braces and return statement. It is redundant

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

solved at 6c63366

@@ -0,0 +1,11 @@
import React from 'react';

const DisplayError = ({ error }) => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The component DisplayError should not be aware of the full error object if it only uses the message

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

solved at 6c63366

return (
<div className="absolute bottom-0 bg-green-100 border border-green-500 text-green-700 rounded p-4 w-full max-w-md text-center mt-4">
<h3 className="font-bold mb-2">Newly Created Movie</h3>
<p><strong>ID:</strong> {data.id}</p>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We could destructure id and name from data since the object data itself it's not used

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

solved at 6c63366

{isSuccess && <DisplaySuccess data={data} />}
</div>
);
}; No newline at end of file
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's leave an empty line at the end of the file

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

solved at 6c63366

import { useFormik } from 'formik';
import * as Yup from 'yup';

const UseSubmitMovieFormik = (onSubmit) => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This hook should start with lower case useSubmitMovieFormik

Copy link
Copy Markdown
Collaborator Author

@Hezanin Hezanin Jul 12, 2024

Choose a reason for hiding this comment

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

hook was deleted but point noted :D

import * as Yup from 'yup';

const UseSubmitMovieFormik = (onSubmit) => {
return useFormik({
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should return the jsx directly instead of adding curly braces and return statement

Ex: const useSubmitMovieFormik = () => useFormik()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

hook was deleted but point noted :D

package.json Outdated
"react": "^18",
"react-dom": "^18"
"react-dom": "^18",
"yup": "^1.4.0"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should remove the "^" because we need the libraries to be stable and not have surprises with breaking changes. let's remove it from all the libraries as well

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

solved at 6c63366

Hezanin added 4 commits July 12, 2024 21:34
Update form field to contain a formik field as to remove the redundant custom implementation;
Update create movie component to include extractions for initial values, validation schema, form
- remove reduntant rename of mutate function and call
- update display error component to not receive error object but error message
- update display success component to destructure id and name
- update package file dependencies to not include '^' before version
Update movie form field for id to type number
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.

2 participants