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
Tasks refactor #208
Tasks refactor #208
Conversation
swarajpure
commented
Feb 1, 2022
•
edited
edited
- New task statuses should be visible in sidebar
- New task statuses should be selectable in a dropdown for a task, such that task status can be changed by clicking the dropdown
- Message in case of no tasks being found
- Refactor logic for filtering tasks according to task type
- Refactor logic after API call to update a task
- Add loader and response after updating a task
- Add ALL tasks in the sidebar such that we support and show the tasks with older task statuses
45225b2
to
40369f8
Compare
325df39
to
62a8e5f
Compare
app/components/task/holder.hbs
Outdated
{{on "change" this.onPercentageChange}} | ||
{{on "change" (fn @onTaskUpdate @task.id)}} | ||
/> | ||
{{#if (eq @task.status 'IN_PROGRESS')}} |
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 we use a known enum here please?
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.
Didn't get this point, couldn't find anything useful on Googling for enum in ember
or enum in handlebars
Can you please provide some resource for this one?
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.
const STATUSES = {
IN_PROGRESS: 'IN_PROGRESS',
}
app/constants/tasks.js
Outdated
COMPLETED: 'completed', | ||
PENDING: 'pending', | ||
}; | ||
const TASK_STATUSES = [ |
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.
I wonder if this should be a map, instead of an array.
If I want to look up if there's a key called TASK_STATUSES.<keyname>
, we have no easy way of doing it anymore. What do we benefit from making this an array here?
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.
we're using this array to show the sidebar options, if we use map, the order would be messed up, right?
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.
@swarajpure We can take the enum list, and make it into an array using Object.keys
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.
My concern with using an object is, will we be able to ensure the order of the keys? If the order is messed up, the list will obtained from Object.keys
will also have different order and the sidebar won't have predictable order. Someone might say "I remember In Progress
was the third option last week, why and how is it changed now?" @ankushdharkar
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.
@swarajpure Good point. So we can do something like this:
const DISPLAY_LIST = [ IN_PROGRESS, COMPLETED, UNASSIGNED ];
Why will this order change?
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.
Made the change
const cleanBody = this.cleanReqBody(taskData); | ||
if (taskData.status || taskData.percentCompleted) { | ||
try { | ||
this.isLoading = true; | ||
const response = await fetch(`${API_BASE_URL}/tasks/self/${taskId}`, { |
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.
This should be moved in ember-data.
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.
i agree but ember data isn't in place yet
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 you create a ticket or mention it for this issue please, so we start integrating it?
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.
@@ -12,7 +12,7 @@ module('Integration | Component | tasks', function (hooks) { | |||
|
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.
Do we need more tests to cover the new feature(s)?
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.
yes, we definitely need tests, there are no tests as of now, since this feature had a tight deadline
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.
@swarajpure Then let's please have a ticket that tracks the follow-up of tests with timeline/ETA
@@ -1,16 +1,44 @@ | |||
<div class="task-card"> | |||
<div class="task-card" tabindex="0"> |
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.
added tabindex
so each task card can be highlighted when traversing via keyboard
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.
Hi @swarajpure
Are we using /tasks/self endpoint to get all the tasks, if yes we cannot make use of it since it does not return all the tasks
Checkout this 👇
https://github.com/Real-Dev-Squad/website-api-contracts/tree/main/tasks#get-tasksself
we can make use of /tasks/username 👇
https://github.com/Real-Dev-Squad/website-api-contracts/tree/main/tasks#get-tasksusername
RESOLVED ⬆️