-
Notifications
You must be signed in to change notification settings - Fork 2
#11 - users crud operations #20
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?
Conversation
EwelinaSkrzypacz
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.
- Please fix checks - all are failed.
- When I want to register, after filling form and click "Register" button I see error
- Where is navigation? All pages should have navigation like here.
- When I'm on user index page and I click "Create user" button I see error in dev console. The same thing happens when I click "Edit button"
-
Add button in navigation to user list - now I had to manually write url to the page
-
Add validation in create/edit form - for example, when user go to create user page and fills nothing, just click "Save" nothing happens - he should see validation messages under each field like "email is required"; also we should validate length of fields. Also email should be unique maybe?
-
Create user form - please fix warning in dev console
- Create user form - when I click "Cancel" button I see error (the same thing happens on edit user form)
- I'm not fan of this solution - we should make custom modals for deleting stuff
- Missing title on browser card
|
|
||
| class UserController extends Controller | ||
| { | ||
| /** |
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.
These comments don't add value to the code and could be removed to improve readability.
| ...tseslint.configs.recommended, | ||
| ...pluginVue.configs["flat/essential"], | ||
| {files: ["**/*.vue"], languageOptions: {parserOptions: {parser: tseslint.parser}}}, | ||
| ]; |
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.
Remember to add empty line at the end of the file
| @@ -0,0 +1,89 @@ | |||
| <template> | |||
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 fix all GitHub errors
resources/js/Pages/User/Create.vue
Outdated
| <div> | ||
| <h1 class="text-2xl mb-4">Create User</h1> | ||
|
|
||
| <!-- User creation form --> |
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 remove all unnecessary comments
| </div> | ||
| </template> | ||
|
|
||
| <script> |
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 move <script> tag at the top of the file, like we have for example in Register.vue file
| }; | ||
| </script> | ||
|
|
||
| <style scoped> |
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 that we can style this using tailwind CSS classes - there is no need to use scope style 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.
Maybe create components for inputs, buttons and all elements that will be often use? Thanks to that we will have consistent design and programming views will be faster. Please talk about that with others.
app/Models/User.php
Outdated
| use Notifiable; | ||
| use HasApiTokens, HasFactory, Notifiable; | ||
|
|
||
| public mixed $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.
Why do we need this?
app/Models/User.php
Outdated
| use HasApiTokens, HasFactory, Notifiable; | ||
|
|
||
| public mixed $id; | ||
| protected $table = 'users'; |
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.
And this?
| methods: { | ||
| createUser() { | ||
| // Logic to handle user creation (e.g., make an API call) | ||
| this.$inertia.post(route('users.store'), this.user); |
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 make it more simply - like we have in Register.vue file (line 16)
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'll be happy to review this PR once all checks have passed and my feedback has been implemented.







I implemented user CRUD operations using the Laravel framework.
I successfully integrated user creation,reading, updating, and deletion functionalities.
I made all operations compatible with Vue.js and Inertia.js and optimized the database connections.