Group resolved comments #2117
Group resolved comments #2117
Conversation
…hat the comment went under a different list so they are not confused.
Codecov Report
@@ Coverage Diff @@
## master #2117 +/- ##
==========================================
- Coverage 92.65% 92.63% -0.02%
==========================================
Files 207 207
Lines 12045 12040 -5
Branches 1764 1747 -17
==========================================
- Hits 11160 11153 -7
- Misses 885 887 +2
Continue to review full report at Codecov.
|
| return {commentSectionOpen: true, scrollToThreadID: initThreadID}; | ||
| }, () => { | ||
| setTimeout(() => this.setState({scrollToThreadID: null}), FLASH_DELAY); |
There was a problem hiding this comment.
I wish we had a way to collapse the setState updater functions from here and highlightThread into a single, atomic transaction. I'm pretty sure that's how it's working anyway and this is much cleaner but still 😛
| return null; // shh, eslint | ||
| }); | ||
| } | ||
| this.scrollToThread(scrollToThreadID); |
| @@ -308,7 +299,8 @@ export default class ReviewsView extends React.Component { | |||
| return null; | |||
| } | |||
|
|
|||
| const resolvedThreads = commentThreads.filter(pair => pair.thread.isResolved).length; | |||
There was a problem hiding this comment.
Hah, should have called this resolvedThreadCount or something before. 👍
| if (threadHolder) { | ||
| threadHolder.map(element => { | ||
| element.scrollIntoViewIfNeeded(); | ||
| return null; // shh, eslint |
There was a problem hiding this comment.
I really need to make a RefHolder::forEach method to make these cases nicer...
| } | ||
| } | ||
|
|
||
| async resolveUnresolveThread(thread) { |
There was a problem hiding this comment.
We keep hitting situations where we need to name methods that toggle something without having a good word for the toggling action. There isn't really a good alternative for this:
resolveOrUnresolveThread()toggleThreadResolution()- ... ?
Description of the Change
Screenshot/Gif
Alternate Designs
I experimented with using filter toggle but the user experience feels clunky, and with the current design, there isn't really a natural placement for the toggle.
Benefits
Resolved comments are moved out of the way, further encouraging the use of review comments as a to-do list behaviour model.
Possible Drawbacks
It might confuse users slightly when the resolved comment gets moved to another location. The highlighting should mitigate this confusion, but the problem can still exist for cases where there are many comments. However, the current implementation of collapsing comments immediately after resolving is probably just as confusing, so I don't see this feature making the experience worse.
Applicable Issues
#2078
Metrics
Tests
TBD: still working on them!
Release Notes
Group resolved review comments into its own list.