Add task-list model #857
Add task-list model #857
Conversation
WenInCode
force-pushed
the
add-task-list-model
branch
from
0cfce37 to
68e9625
Compare
joshsmith
left a comment
joshsmith
left a comment
There was a problem hiding this comment.
Looking good so far! 👍
|
This is my first time working with mirage, so I mostly just based it off of the other models. Ready for code review though. |
joshsmith
left a comment
joshsmith
left a comment
There was a problem hiding this comment.
Some notes here. Can discuss further if needed.
app/models/task-list.js
Outdated
|
|
||
| export default Model.extend({ | ||
| name: attr('string'), | ||
| position: attr('number'), |
There was a problem hiding this comment.
I think you can just do attr() on both without arguments.
There was a problem hiding this comment.
We may want to wait on naming convention until this settles in code-corps/code-corps-api#557
@green-arrow are discussing rank vs order.
One thing that is missing here, though, is the distinction, which will require some manual work in the mirage routes for the task. We'll need to make it so that position can be set on the model, but it's a virtual attribute on the server that then gets turned into rank or order, which is a completely different number.
This is a little odd but setting and getting are not the same thing, or even the same property.
app/models/task.js
Outdated
| insertedAt: attr('date'), | ||
| markdown: attr(), | ||
| number: attr('number'), | ||
| position: attr('number'), |
joshsmith
added
needs submitter response
and removed
Priority: High
needs code review
labels
|
@joshsmith updated, not sure if these were the changes you were looking for |
joshsmith
left a comment
joshsmith
left a comment
There was a problem hiding this comment.
Couple minor comments.
| export default Factory.extend({ | ||
| order() { | ||
| return (this.position || 0) * 3; | ||
| }, |
There was a problem hiding this comment.
I would make this multiplier significantly larger, like 100.
app/models/task.js
Outdated
| number: attr('number'), | ||
| position: attr('number'), | ||
| order: attr(), | ||
| position: attr(), |
There was a problem hiding this comment.
I would set position below the others separately (see stripe-platform-card) and write that it is a virtual, write-only attribute. Would also document that order is read-only.
|
@WenInCode LGTM! |
|
great - thanks @joshsmith |
WenInCode
force-pushed
the
add-task-list-model
branch
from
6e67daf to
7a265e5
Compare
What's in this PR?
task-listmodelproject&taskmodelsReferences
Fixes #855