-
Notifications
You must be signed in to change notification settings - Fork 15
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
add task reassignment functionality #195
base: develop
Are you sure you want to change the base?
Conversation
src/api/tasks.js
Outdated
} else { | ||
// Add t to its parent's subtasks | ||
taskMap[t.parent_task_gid].subtask_gids.push(t.gid); | ||
taskMap[t2.parent_task_gid].subtask_gids.push(t2.gid); |
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.
Glad you caught this. We should be able to replace the GET
to /postgrest/tasks
(see getTasksReq
) with a POST
to /postgrest/rpc/
, to call subtasks_recursive_flat
(see
pursuance/db/sql/migration0003.sql
Line 3 in 820af87
CREATE FUNCTION subtasks_recursive_flat(parent_gid text, subtask_pursuance_id integer) RETURNS TABLE(gid text, pursuance_id integer, id integer, title text, title_enc text, deliverables text, deliverables_enc text, assigned_to text, assigned_to_pursuance_id integer, due_date timestamp WITH time zone, created timestamp WITH time zone, parent_task_gid text) AS $$ |
...but it might actually be faster to just do what you've done here -- iterate twice -- rather than do a more complex DB query to get things in the right order; I'm not sure.
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.
Looking pretty great, but please make a couple tweaks as suggested 👍
@@ -167,7 +224,8 @@ class RawTask extends Component { | |||
<div className="toggle-ctn"> | |||
{this.getTaskIcon(task, showChildren)} | |||
</div> | |||
<div className="task-row-ctn"> | |||
{connectDropTarget(connectDragSource( | |||
<div className="task-row-ctn" style={{ backgroundColor: canDrop && isOver ? '#50b3fe' : '' }}> |
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.
For complicated security reasons we are trying to avoid inline styling. Could you replace this with something like className={"task-row-ctn " + (canDrop && isOver ? 'highlight-task') : ''}
?
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.
np
withRouter, | ||
connect( | ||
({ pursuances, user, users, currentPursuanceId, autoComplete, rightPanel }) => | ||
({ pursuances, user, users, currentPursuanceId, autoComplete, rightPanel }), { |
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 is the most readable way I've seen to do this; sweet 👍 .
const oldParentSubtaskGids = oldParentTask.subtask_gids.filter( | ||
gid => gid !== taskGid | ||
); | ||
const newSubtaskGids = [...newParentTask.subtask_gids, taskGid]; |
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.
So that what redux shows right after the drag-and-drop occurs is the same as what the user will see after a refresh, technically the subtask_gids
should be ordered by created
and then by id
. Think it'd be too slow to do that sorting 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.
Sure :-)
} | ||
}) | ||
}); | ||
|
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.
Hell yeah! 🎉
}; | ||
|
||
const taskTarget = { | ||
canDrop(props, monitor) { |
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.
@colinfruit Adding console.log("canDrop called at", new Date())
here shows that canDrop
is called hundreds of times even if the div
that the user's mouse is hovering over doesn't change.
Think there's some way to make this more efficient? It works great but it's very slow and I'm trying to figure out why.
src/reducers/tasksReducer.js
Outdated
newSubtasks.sort(function(t1, t2) { | ||
const t1Date = new Date(newMap[t1].created); | ||
const t2Date = new Date(newMap[t2].created); | ||
if (t1Date === t2Date) { |
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.
@colinfruit Is this the right comparison? We just want to return 0
immediately if they're the same, yes?
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 thought we were sorting by created then by id per the earlier comment. Did I misunderstand?
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.
@colinfruit Ooh I see, no you're right. Can you rename t1
and t2
to gid1
and gid2
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.
I lost track of what was a task v. a date v. a gid, which is why I was confused.
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.
👍 will rename
|
||
newSubtasks.sort(function(gid1, gid2) { | ||
const t1Date = new Date(newMap[gid1].created); | ||
const t2Date = new Date(newMap[gid2].created); |
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.
@colinfruit Efficiency idea:
const t1 = newMap[gid1];
const t2 = newMap[gid2];
t1.created_parsed = t1.created_parsed || new Date(t1.created);
t2.created_parsed = t2.created_parsed || new Date(t2.created);
so that there's no need to parse any task's created
timestamp twice.
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.
nice, created_parsed a field? Or is it on the todo list?
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.
It's a field that we'd be creating just to make this more efficient.
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 point is that, the second time that one of these tasks's created
timestamp goes to be parsed, it'll reuse created_parsed
rather than parsing the created
field again and creating a Date
object out of it.
No description provided.