vue_dynamic-list-of-posts #49
Conversation
There was a problem hiding this comment.
Great job overall! 😊 There are a few areas that could be improved, but none of them are critical. They are more about following best practices and improving code readability. Keep up the good work and always strive to improve!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/api/post.js
Outdated
| return client.get("/posts"); | ||
| }; | ||
|
|
||
| export const createPosts = ({ body, title, userId }) => { |
There was a problem hiding this comment.
It seems there is a minor naming issue here. Your function is named createPosts, but it appears to create only a single post. Consider renaming this function to createPost for clarity.
src/api/post.js
Outdated
| }; | ||
|
|
||
| export const deletePost = (postId) => { | ||
| return client.delete(`https://mate.academy/students-api/posts/${postId}`); |
There was a problem hiding this comment.
You are using a full URL in the deletePost method, while in other methods you use relative URLs. This can lead to errors if the base URL changes. It would be better to keep consistency and use a relative URL here as well, like in other methods.
| }; | ||
|
|
||
| export const registrationUser = (email, name) => { | ||
| return client.post(`/users?email=${email}`, { email, name }); |
There was a problem hiding this comment.
It seems like there is a misunderstanding in how to use HTTP methods. The POST method is used to send data to the server to create a new resource. The query parameters (like email=${email}) are usually used with GET requests to filter the returned data. In your case, you are trying to create a new user, so the data should be sent in the body of the request, not in the query parameters. The correct usage would be client.post('/users', { email, name }).
src/main.js
Outdated
| addPostList(state, posts) { | ||
| if (Array.isArray(posts)) { | ||
| state.postList = [...state.postList, ...posts]; | ||
| } else { | ||
| state.postList.push(posts); | ||
| } |
There was a problem hiding this comment.
This mutation addPostList can be improved. Currently, it accepts both an array of posts and a single post. It would be better to have two separate mutations, one for adding a single post and another for adding multiple posts. This would make the code more readable and less error-prone.
src/main.js
Outdated
| updatePost(state, newPost) { | ||
| const index = state.postList.findIndex( | ||
| (post) => post.id === newPost.id | ||
| ); | ||
|
|
||
| state.postList.splice(index, 1, newPost); | ||
| }, |
There was a problem hiding this comment.
In the updatePost mutation, you're not checking if the post exists in the list before replacing it. If findIndex doesn't find the post, it returns -1, and splice will then remove the last item in the array. To prevent this, you should check if the post exists before updating it.
src/main.js
Outdated
| deletePost(state, postId) { | ||
| const index = state.postList.findIndex( | ||
| (post) => post.id === postId | ||
| ); | ||
|
|
||
| state.postList.splice(index, 1); | ||
| }, |
There was a problem hiding this comment.
In the deletePost mutation, you're not checking if the post exists in the list before deleting it. If findIndex doesn't find the post, it returns -1, and splice will then remove the last item in the array. To prevent this, you should check if the post exists before deleting it.
| try { | ||
| return JSON.parse(value); | ||
| } catch { | ||
| return null; |
There was a problem hiding this comment.
It's good that you're catching exceptions when parsing JSON, but it would be helpful to log the error message to the console for debugging purposes. You can do this by catching the error in a variable (e.g., catch (e)), and then logging it (console.error(e)).
| @@ -0,0 +1,13 @@ | |||
| export const getLocaleStorage = (key) => { | |||
| const value = window.localStorage.getItem(key); | |||
There was a problem hiding this comment.
It's a good practice to check if the localStorage is available before using it. This is because some browsers may not support localStorage, or the user may have disabled it. You can do this by checking if (window.localStorage) {...} before trying to get the item.
src/utils/setLocaleStorage.js
Outdated
| const jsonData = JSON.stringify(data); | ||
|
|
||
| window.localStorage.setItem(key, jsonData); | ||
| } catch (error) {} |
There was a problem hiding this comment.
It's generally a bad practice to have an empty catch block. Even if you think that an error here should not break the application, you should at least log it to the console. This will help you to debug issues and understand what went wrong.
[the working version] (https://vazilx.github.io/vue_dynamic-list-of-posts/)