-
Notifications
You must be signed in to change notification settings - Fork 247
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
feat(ui): Add function to toggle reactions from timeline #2118
Conversation
toggle_reaction
function to timeline
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2118 +/- ##
==========================================
+ Coverage 76.12% 76.32% +0.19%
==========================================
Files 163 163
Lines 17303 17513 +210
==========================================
+ Hits 13172 13366 +194
- Misses 4131 4147 +16
☔ View full report in Codecov by Sentry. |
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.
Very rough first review. Thanks for adding a bunch of tests!
Thanks for the review @jplatte - I have addressed the issues |
…ny/timeline-reactions
d52869f
to
6d15747
Compare
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.
Small code comments here and there, but with the main locking issue resolved, that looks good to me! Will let @jplatte handle the final approval.
match result { | ||
ReactionAction::None => {} | ||
ReactionAction::SendRemote(_) | ReactionAction::RedactRemote(_) => { | ||
// Remember the new in flight request | ||
state.in_flight_reaction.insert(annotation.into(), reaction_state); | ||
} | ||
}; |
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.
nit: This match
could be avoided if we duplicated line 277 in the two places in the above match. I would prefer this slightly better, but it's personal taste, so feel free to dismiss :)
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 added two matches to make it a bit clearer what is being done (i.e. first calculate the result, then update the state) and avoid the side effect getting lost amongst the other logic.
Given the function is already pretty long I'd probably err on the side of clarity if that makes sense to you also?
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.
One last thing, I think then we can merge.
Great work @jonnyandrew! |
Changes
Current limitations
Does not include
Context
Tasks
toggle_reaction
function to timelineSigned-off-by: