Implement adding notes to timespans in the backend #81
Conversation
Codecov Report
@@ Coverage Diff @@
## master #81 +/- ##
==========================================
+ Coverage 92.08% 92.09% +0.01%
==========================================
Files 70 70
Lines 1326 1328 +2
==========================================
+ Hits 1221 1223 +2
Misses 71 71
Partials 34 34
Continue to review full report at Codecov.
|
|
Is there a reason why updates are done by deleting and recreating the timespan? |
model/timespan.go
Outdated
| @@ -12,6 +12,7 @@ type TimeSpan struct { | |||
| OffsetUTC int | |||
| UserID int `gorm:"type:int REFERENCES users(id) ON DELETE CASCADE"` | |||
| Tags []TimeSpanTag | |||
| Notes *string | |||
There was a problem hiding this comment.
Can we make this not nullable? In the UI we don't need to differentiate between nil and empty string.
There was a problem hiding this comment.
Also not nullable in the graphql types?
timespan/copy.go
Outdated
| @@ -17,5 +17,5 @@ func (r *ResolverForTimeSpan) CopyTimeSpan(ctx context.Context, id int, start mo | |||
| return nil, fmt.Errorf("time span with id %d does not exist", id) | |||
| } | |||
|
|
|||
| return r.CreateTimeSpan(ctx, start, end, tagsToInputTag(old.Tags)) | |||
| return r.CreateTimeSpan(ctx, start, end, tagsToInputTag(old.Tags), nil) | |||
There was a problem hiding this comment.
I'm not sure, but shouldn't this copy the note as well?
There was a problem hiding this comment.
I guess you can make an argument for both options. I added it
| @@ -12,6 +12,7 @@ type TimeSpan struct { | |||
| OffsetUTC int | |||
There was a problem hiding this comment.
Yeah, feel free to do the UI part as well. I'd say the UI could look like this, the notes should be defined as Markdown and then are rendered like this inside the ui.
There was a problem hiding this comment.
The note can then be edited by clicking it.
There was a problem hiding this comment.
Do you want to include this in the current pr or another?
There was a problem hiding this comment.
I'll see if I can come up with something. Currently this PR breaks the client due to the required notes input field
|
I've added a basic implementation. I'm a complete react noob. This is the first time I've done any react whatsoever... There's one issue with the current debounce. If the user changes some other value during that period, the change is overwritten when the debounce completes. I'm not sure if you can live with that, or you want to change it, but updating the notes on each keystroke doesn't sound right... |
|
Alright, I'll hopefully have a look at this on the weekend. |
|
@simhnna This looks good so far, as I'm currently a little busy I'll postpone this another 2 weeks then I'm on vacation and have a little more free time. |
|
Any news here? I'm excited for this feature to come :) |
jmattheis
force-pushed
the
add-notes-to-backend
branch
from
5861604
to
46c798c
Compare
|
@simhnna Would you mind taking a quick look? This missing feature is essentially the only thing that hinders me from switching from Toggl to Traggo :) |
|
I'll probably merge it tomorrow or on Saturday when I find some time to test and create a release. |
This implements the backend part of #74
Should I just try updating the frontend as well, or do you have something specific in mind?