palash-ccm
commented
6 months ago Hi Ethan,
Thank you for the feedback. It is very helpful, I will implement these changes immediately.
Get Outlook for Androidhttps://aka.ms/AAb9ysg
From: ethan-ccm @.> Sent: Friday, January 19, 2024 4:34:29 pm To: palash-ccm/VueBasicsAssignment @.> Cc: Subscribed @.***> Subject: [palash-ccm/VueBasicsAssignment] Feedback (Issue #1)
EXTERNAL EMAIL: DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/APICalls.vue#L1-L15 I think your decision to place your functions for dealing with the API in a separate file was a good one, but it should be a typescript file instead of a .vue file with a script tag. Additionally, error handling on all functions making API requests would be nicer than just on the GET request. https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/UI/BaseCard.vue#L1-L19 I can see what you were going for here, and it's perfectly acceptable to have "dumb" components that have no state and are only for organizing your UI, but in this case you should probably just use a div with a class here instead of a component since your component is just a div with some styling. https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L11 I'm really not sure what this label is doing here https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L23 Better to have types capitalized but not a big deal https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L30 Empty arrays should specify their type ideally https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L35-L47 There's probably a nicer way to check this here than JSON.stringify, but I also think this might be simpler if you had a separate boolean ref for each filter maybe. Also if you notice, you only ever assign to selectedTasks by deriving some value from tasks this is a good hint that you might want a computed here instead of a ref which would also likely simplify things quite a bit https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L74-L95 I can see how in some cases organizing things like this might simplify your code, and I do like that your functions that deal with the API aren't also modifying state but here it seems like it's extra complication for not much gain. But it's also just a preference thing, and I generally dislike functions that do a bunch of different things based on a string argument being passed in. This does seem similar to useReducer / Redux in react or the elm architecture that those were inspired by, which are all fine ways of organizing code, but I think a simple example like this might not need something so structured.
— Reply to this email directly, view it on GitHubhttps://github.com/palash-ccm/VueBasicsAssignment/issues/1, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BFJXFIHQSPIM5L35USK3LRLYPLRGDAVCNFSM6AAAAABCCSCC52VHI2DSMVQWIX3LMV43ASLTON2WKOZSGA4TCNBVGQ2TSOI. You are receiving this because you are subscribed to this thread.Message ID: @.***>
https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/APICalls.vue#L1-L15 I think your decision to place your functions for dealing with the API in a separate file was a good one, but it should be a typescript file instead of a .vue file with a script tag. Additionally, error handling on all functions making API requests would be nicer than just on the GET request. https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/UI/BaseCard.vue#L1-L19 I can see what you were going for here, and it's perfectly acceptable to have "dumb" components that have no state and are only for organizing your UI, but in this case you should probably just use a div with a class here instead of a component since your component is just a div with some styling. https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L11 I'm really not sure what this label is doing here https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L23 Better to have types capitalized but not a big deal https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L30 Empty arrays should specify their type ideally https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L35-L47 There's probably a nicer way to check this here than JSON.stringify, but I also think this might be simpler if you had a separate boolean ref for each filter maybe. Also if you notice, you only ever assign to
selectedTasksby deriving some value fromtasksthis is a good hint that you might want a computed here instead of a ref which would also likely simplify things quite a bit https://github.com/palash-ccm/VueBasicsAssignment/blob/b1d3ae731687ce4c781ce6e122514ce78733d54b/src/components/HelloWorld.vue#L74-L95 I can see how in some cases organizing things like this might simplify your code, and I do like that your functions that deal with the API aren't also modifying state but here it seems like it's extra complication for not much gain. But it's also just a preference thing, and I generally dislike functions that do a bunch of different things based on a string argument being passed in. This does seem similar touseReducer/ Redux in react or the elm architecture that those were inspired by, which are all fine ways of organizing code, but I think a simple example like this might not need something so structured.