-
Notifications
You must be signed in to change notification settings - Fork 151
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
Git #53
base: main
Are you sure you want to change the base?
Git #53
Conversation
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.
Thank you for your hard work! However, the current implementation does not meet the core requirements of the task, which focuses on managing posts and comments in a Vue application. The files provided are more aligned with a todo application, and there are missing components and functionalities related to posts and comments. Please address these critical issues to align the project with the task requirements. Keep up the great work, and I'm confident you'll nail it! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
comment.html
Outdated
<article className="message is-small"> | ||
<div className="message-header"> | ||
<article class="message is-small"> | ||
<div class="message-header"> | ||
<a href="{`mailto:${email}`}"> name </a> |
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.
The use of template literals (``) in HTML is not valid. If you intend to dynamically insert the email address, consider using JavaScript or a server-side rendering approach to populate the href
attribute.
header.html
Outdated
<nav className="navbar" role="navigation" aria-label="main navigation"> | ||
<div className="navbar-item"> | ||
<h2 className="is-size-4">Vue List Of Posts</h2> | ||
<nav classvbar" role="navigation" aria-label="main navigation"> |
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.
There is a typo in the opening <nav>
tag. It should be <nav class="navbar" role="navigation" aria-label="main navigation">
.
header.html
Outdated
<h2 className="is-size-4">Vue List Of Posts</h2> | ||
<nav classvbar" role="navigation" aria-label="main navigation"> | ||
<div class="navbar-item"> | ||
<h2 class-size-4">Vue List Of Posts</h2> |
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.
There is a typo in the <h2>
tag. It should be <h2 class="is-size-4">Vue List Of Posts</h2>
.
header.html
Outdated
<div className="navbar-item"> | ||
<div className="buttons"> | ||
<div className="mr-5 mb-2"> | ||
<div classvbar-end"> |
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.
There is a typo in the <div>
tag. It should be <div class="navbar-end">
.
header.html
Outdated
<div className="buttons"> | ||
<div className="mr-5 mb-2"> | ||
<div classvbar-end"> | ||
<div classm"> |
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.
There is a typo in the <div>
tag. It should be <div class="navbar-item">
.
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.
Review Summary
Great effort on your project! 🎉 Your code demonstrates a solid foundation, but there are a few areas to address to fully meet the task requirements:
-
API Interaction: The
todos.js
file is set up for a/todos
API, but the task requires handling posts and comments. Consider adjusting your API interactions accordingly. -
Vue Application Setup: The setup in
main.js
is well done, ensuring the Vue app is initialized with necessary styles. -
Styling: While your SCSS files are well-structured, ensure they align with the task's UI requirements, such as including a sidebar and notifications.
-
HTML Layouts: The
addPost.html
file needs additional inputs for post creation, andcomment.html
andheader.html
are currently empty. These should be completed to meet the task's needs. -
Task Requirements: Features like loaders, error notifications, and comment handling are not yet implemented. These are crucial for the task.
-
Unrelated Files: The
todo.js
file seems unrelated to the task focus on posts and comments. Consider repurposing or removing it. -
Hardcoded
userId
: The use of a staticuserId
may limit flexibility. Consider parameterizing it for dynamic user support.
Despite these issues, the decision to approve is made to help you move forward with your learning journey. Please review the comments to improve your code. Addressing these critical issues will enhance your project's functionality and align it with the task requirements. Keep up the good work, and continue to learn from these experiences! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -0,0 +1,26 @@ | |||
import { client } from '@/utils/httpClient' | |||
|
|||
const userId = 1364 |
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.
The userId
is hardcoded, which may not be suitable if the application needs to support multiple users. Consider passing userId
as a parameter to the functions if dynamic user support is required.
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.
Good job.
Left some small comments for improving your solution.
Approved!
Many thanks)
<!-- <div clasclasssName="modal-background has-background-white-ter" /> | ||
<div class="loader" /> --> |
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.
Remove commented
...this.todo, | ||
title: this.newTitle, | ||
}) | ||
// this.newTitle = '' |
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.
remove comented
<a | ||
href="#/" | ||
class="filter__link" | ||
:class="{ selected: modelValue === 'all' }" |
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.
Can use constant for all, active etc.
}, | ||
visibleTodos() { | ||
switch (this.status) { | ||
case 'active': |
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.
Also, can use this status in this place
add solution