-
Notifications
You must be signed in to change notification settings - Fork 200
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
Scroll to first highlighted annotation when loaded #6337
Conversation
1057e63
to
2c27fd8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6337 +/- ##
=======================================
Coverage 99.43% 99.43%
=======================================
Files 271 271
Lines 10217 10223 +6
Branches 2419 2420 +1
=======================================
+ Hits 10159 10165 +6
Misses 58 58 ☔ View full report in Codecov by Sentry. |
// Scroll to the first highlighted annotation, unless creating/editing another | ||
// annotation | ||
useEffect(() => { | ||
const [firstHighlightedAnnotation] = highlightedAnnotations; |
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 doesn't take account of the current sort order in the thread list. I guess it will scroll to whichever update came in first, if that is the order in which highlights naturally are sorted in the store?
One easy way to make this feel predictable might be to sort by last update time and scroll to the oldest (ie. the first update the user hasn't seen yet) or alternatively the newest.
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 only intention here was to make sure the user sees at least one of the highlighted annotations, without worrying too much which one is it.
But we could definitely pick one by date, as that should not complicate this too much.
if (!editing && firstHighlightedAnnotation) { | ||
setScrollToId(firstHighlightedAnnotation); | ||
} | ||
}, [editing, highlightedAnnotations]); |
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 started with an algorithm where scrolling happens to the first highlighted annotation, because it was a pretty simple logic to implement.
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.
LGTM. One issue I noticed in testing is that although the UI will scroll if needed, it won't switch tabs from annotations <-> notes or vice-versa. I suggest filing a follow-up issue for that.
Part of #6255
This PR updates
ThereadList
so that it scrolls to the first highlighted annotation once pending updates have been loaded, as long as the user does not have any draft.This is a simple approach to scroll into just-loaded annotations, however, we should probably scroll into the closest one based on the active scroll. I will try to address that separately.
Testing steps
If you do the same while editing/creaating an annotation, the scrolling should not happen, unless you cancel all drafts before the 5-second timeout where annotations are highlighted, is finished.