Skip to content
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

Made Changes Required For CSOC Vue Tasks #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Samar1110
Copy link

Name : Samar Singh Randhawa
Roll No : 21095101

  • Login
  • auth_required.js and no_auth_required.js
  • Add Task
  • Get Tasks
  • Edit Task
  • Delete Task
  • UI Improvement

My Github Repo - Repo_Link

Copy link
Member

@IamEzio IamEzio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Samar1110! Overall a nice PR. The application is working fine and great use of toast. However, I do not see the deploy link in the issue description.
Key takeaways:

  • Always make a PR from a feature branch.
  • There was a task for UI improvement but I do not see any new features added.
  • Noticed some console errors while running the application. They appeared when the logout button was clicked. PTAL

@@ -2,6 +2,7 @@
<aside class="mx-auto flex justify-between mt-24 px-4">
<label for="add task" class="flex-1">
<input
v-model.trim="newTask"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch on using trim.

// this.$router.go()
})
.catch(() => {
this.$toast.error('Task not Added Succesfully')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good error callback would be to ask the user to try again.

const path = context.route.fullPath
const token = context.store.getters.token

const newLocal = token != null && path !== '/'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use JS naming convention for naming a boolean variable.

if (this.todos[_index].title === '') {
this.$toast.error('Enter a Task in the Edit part...')
return
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A check could be added to make sure that the value of the todo title is changed and that it is not the same as the original one. This would prevent sending void calls to the backend.

@@ -2,6 +2,7 @@
"compilerOptions": {
"target": "ES2018",
"module": "ESNext",
"jsx": "react",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

@Samar1110
Copy link
Author

Actually when I was deploying my site using pnpm run build it showed some error by which later on my site was not working, so i did not deployed it
Shall i deploy it now ?

@IamEzio
Copy link
Member

IamEzio commented Jul 24, 2022

Actually when I was deploying my site using pnpm run build it showed some error by which later on my site was not working, so i did not deployed it Shall i deploy it now ?

You should definitely give it another try. If you are still not able to do it, feel free to post the error log here or on the discord so that we can help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants