Csoc week3 #7
Csoc week3 #7
Conversation
abhishek-sultaniya
commented
Jun 29, 2022
•
abhishek-sultaniya
commented
There was a problem hiding this comment.
Overall a good PR. Nice work!
The functionality seems to be working fine -- however, there are a few glitches, and CSS is to be improved. Some important takeaways:
- Remove console log statements before submitting PR.
- Always make changes in a separate feature branch and keep your origin/main the same as upstream/main.
- Run an indentation check on the whole PR. Bad indentation makes the code difficult to read and understand.
- Toasts raised to the User should be apt and descriptive of the situation. Prompt hum wherever necessary to try again when an error occurs.
- Work on building a better UI.
| @@ -3,6 +3,7 @@ | |||
| <label for="add task" class="flex-1"> | |||
| <input | |||
| type="text" | |||
| v-model.trim="inputText" | |||
| @@ -35,6 +36,7 @@ | |||
| hover:border-transparent | |||
| rounded | |||
| " | |||
| id="addbtn" | |||
There was a problem hiding this comment.
Use camel case naming convention.
There was a problem hiding this comment.
Ok,next time I will keep it in mind.
| /** | ||
| * @todo Complete this function. | ||
| * @todo 1. Send the request to add the task to the backend server. | ||
| * @todo 2. Add the task in the dom. | ||
| * @hint use emit to make a event that parent can observe | ||
| */ | ||
| console.log("debyg"); | ||
| console.log(this.inputText); |
There was a problem hiding this comment.
Remove all console log statements before submitting PR.
Ditto below.
There was a problem hiding this comment.
Ok,Should i comment all console logs or erase it?
Sometimes I have to use it for debugging.
There was a problem hiding this comment.
It's better to remove all console log statements before submitting PRs. When you submit code to a larger repo which is open-source, you don't want these unnecessary statements to sneak into the codebase.
|
|
||
| if(this.inputText=='') | ||
| { | ||
| this.$toast.error('Cannot Add empty Todo'); |
There was a problem hiding this comment.
Fix indentation and capitalize the error message properly. Ditto below
There was a problem hiding this comment.
From next time I would use prettier.Will that be fine?
I will keep it in mind.
Thanks.
There was a problem hiding this comment.
Yeah, using prettier is fine. However, a habit of writing clean code goes a long way.
| this.$emit('newTask') | ||
| }) | ||
| .catch((err) => { | ||
| this.$toast.error("Unable to add task!..") |
There was a problem hiding this comment.
A good error callback would be to prompt the user to try again later.
| @@ -108,37 +117,60 @@ import { defineComponent } from '@nuxtjs/composition-api' | |||
| import addTask from '~/components/addTask.vue' | |||
|
|
|||
| export default defineComponent({ | |||
| middleware: 'auth', | |||
| }, | ||
|
|
||
| getname(){ |
There was a problem hiding this comment.
Follow proper naming conventions.
| */ | ||
| this.loading = true; | ||
| const headers={ | ||
| Authorization: 'Token ' + this.$store.getters.token |
| @@ -147,25 +179,69 @@ export default defineComponent({ | |||
| * @todo 1. Send the request to update the task to the backend server. | |||
| * @todo 2. Update the task in the dom. | |||
| */ | |||
| updateTask(_index, _id) {}, | |||
| async updateTask(_index, _id) { | |||
There was a problem hiding this comment.
The method should have added some checks to ensure that the new ToDo isn't empty and is not the same as the original one. Accordingly, the necessary toasts should be raised to inform the user.
| } | ||
| $toast.info('Please wait...') | ||
|
|
||
| $axios |
| this.$toast.error("Error!..") | ||
| }) | ||
| } | ||
| this.todos[_index].editing = !this.todos[_index].editing; |
There was a problem hiding this comment.
Why are you adding this here? This is already present on line 199. This might be the reason you are facing difficulties while updating todos.
There was a problem hiding this comment.
Will look it,by mistake during trial I might have added it here.
