-
Notifications
You must be signed in to change notification settings - Fork 0
Users forms #18
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
base: main
Are you sure you want to change the base?
Users forms #18
Conversation
# Conflicts: # public/build/manifest.json
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.
Pull request overview
This PR adds user management functionality to demonstrate form usage in the application, introducing user editing capabilities and improving the table component structure.
Key Changes:
- Implements user CRUD routes under
/adminprefix with edit and show pages - Refactors
UserTablecomponent to be reusable and moves the example page to a standard view - Enhances the callout-error component to automatically display Laravel validation errors
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| routes/web.php | Adds /admin routes for user management (list, show, edit) |
| routes/examples.php | Converts table example route from Livewire component to view |
| resources/views/livewire/users/user-update.blade.php | New user edit form with name and email fields |
| resources/views/livewire/users/user-table.blade.php | Refactors table to be reusable component, adds edit navigation |
| resources/views/livewire/users/user-show.blade.php | New user detail view page |
| resources/views/examples/table.blade.php | New example page that embeds UserTable component |
| resources/views/examples/errors.blade.php | Updates error example to use new callout-error API |
| resources/views/components/cds/callout-error.blade.php | Enhances to automatically display validation errors from Laravel |
| public/build/manifest.json | Build artifact with updated asset hashes |
| public/build/assets/app-ppwW4VC5.css | Removed old build artifact |
| app/Livewire/Users/UserUpdate.php | New Livewire component for user editing with validation |
| app/Livewire/Users/UserShow.php | New Livewire component for displaying user details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
inaydich
left a comment
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.
Let's discuss the directories structure in the meeting. If this is the team project then we need to agree on the structure and file naming in within a team. This looks like a diversion form what we discussed last time.
app/Livewire/Users/UserUpdate.php
Outdated
| #[Layout('components.layouts.app', [ | ||
| 'title' => 'Edit User', | ||
| ])] | ||
| class UserUpdate extends Component |
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 would name this component UserEdit to match the route, The route says 'edit'
| @@ -0,0 +1,13 @@ | |||
| <x-layouts.app title="CD Laravel Base" subtitle="Custom Development"> | |||
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 think having this file is confusing. It is just extra layer to call user-table blade.
| <x-cds.button :href="route('admin.users')"> | ||
| Back to Users | ||
| </x-cds.button> | ||
| </x-layouts.main-article> |
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 think we can improve the workflow here. A user needs to do several extra clicks to see the results.
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.
Agreed. I really just wanted to get a "display item" kind of page in our examples so we can discuss the general solution we want to try for on lists + details. I think this is UX + design as much as anything.
| Cancel | ||
| </x-cds.button> | ||
| </form> | ||
| </x-layouts.main-article> |
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 think user-edit would be better name for this blade/component since it matches the route.
Also moving this form to the modal dialog would improve user experience with editing data workflow.
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've updated the component names. Personally I'm still kind of fussing over the names, since Laravel seems to like different words in different places for these functions. And I agree that in this example user-edit is fine.
routes/web.php
Outdated
| return view('welcome'); | ||
| })->name('home'); | ||
|
|
||
| Route::prefix('/admin')->group(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.
I think adding staying with simple routes would be better example. These are just examples and not the real application. Also unfortunately most of FCS applications will manage user roles and permissions via RoleManagemnt app
Adds the ability to edit users as a way to demonstrate form usage.