-
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
Code review. #15
Comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When installing the app I faced some problem and found some missing steps.
-Composer update will install/update all dependency.
Missing : copy .env.example .env
Double install of laravel/passport. It's already inside your composer JSON. Don't need separate installation.
php artisan migrate does not have --seed included
-After seed super admin is not created in the database.
UI
Use movie slug to show movies, now showing numbers
Pretty url for seo > Actors URL can be improved
Why contact us on every page? Create separate contact us page
Change login Form Fields FIRST_NAME?
Route
-Why Duplicate
Route::get('/user/reviews', 'ClientController@reviews');
Controller
-Refactor code Dashboard\mostly to all validate
$attributes = $request->validate([
'name' => 'required|string|max:30|min:3|unique:actors',
'dob' => 'required|date',
'overview' => 'required|string',
'biography' => 'required|string',
'avatar' => 'required|image',
'background_cover' => 'required|image',
]);
convert to request class/service
or create a function like
and use, this will return an array which is validated. All extra request field will be exempt. ex: $attribute don't have $request->password values. you have to add it later.
$attributes = $this->validate();
The text was updated successfully, but these errors were encountered: