Skip to content
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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Scroll to first highlighted annotation when loaded #6337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/sidebar/components/ThreadList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,18 @@ export default function ThreadList({ threads }: ThreadListProps) {
}
}, [store, newAnnotationTag]);

const editing = store.countDrafts() > 0;
const highlightedAnnotations = store.highlightedAnnotations();

// Scroll to the first highlighted annotation, unless creating/editing another
// annotation
useEffect(() => {
const [firstHighlightedAnnotation] = highlightedAnnotations;
Copy link
Member

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.

Copy link
Contributor Author

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]);
Copy link
Contributor Author

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.


// Effect to scroll a particular thread into view. This is mainly used to
// scroll a newly created annotation into view.
useEffect(() => {
Expand Down
24 changes: 22 additions & 2 deletions src/sidebar/components/test/ThreadList-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
} from '@hypothesis/frontend-testing';
import { mount } from 'enzyme';
import { act } from 'preact/test-utils';
import sinon from 'sinon';

import ThreadList from '../ThreadList';
import { $imports } from '../ThreadList';
Expand Down Expand Up @@ -54,6 +55,8 @@ describe('ThreadList', () => {
fakeStore = {
setForcedVisible: sinon.stub(),
unsavedAnnotations: sinon.stub().returns([]),
countDrafts: sinon.stub().returns(0),
highlightedAnnotations: sinon.stub().returns([]),
};

fakeTopThread = {
Expand Down Expand Up @@ -177,10 +180,27 @@ describe('ThreadList', () => {
addNewAnnotation(wrapper, fakeTopThread.children[3].annotation);

// The third thread in a collection of threads at default height (200)
// should be at 600px. This setting of `scrollTop` is the only externally-
// observable thing that happens here...
// should be at 600px. This setting of `scrollTop` is the only
// externally-observable thing that happens here...
assert.calledWith(fakeScrollTop, 600);
});

it('should do nothing for highlighted annotations while creating/editing', () => {
fakeStore.highlightedAnnotations.returns(['t2', 't3']);
fakeStore.countDrafts.returns(10);
createComponent();

assert.notCalled(fakeScrollTop);
});

it('should set the scroll container `scrollTop` to first highlighted annotation', () => {
fakeStore.highlightedAnnotations.returns(['t2', 't3']);
createComponent();

// The first thread in a collection of threads at default height (200)
// should be at 200px.
assert.calledWith(fakeScrollTop, 200);
});
});

/**
Expand Down