feat(scrolling): add scroll to annotation#463
Conversation
mickr
force-pushed
the
scroll-to-active-annotation
branch
from
c241294 to
075f866
Compare
| builder.addCase(setActiveAnnotationIdAction, (state, { payload: annotationId }) => annotationId), | ||
| builder | ||
| .addCase(setActiveAnnotationIdAction, (state, { payload: annotationId }) => annotationId) | ||
| .addCase(createAnnotationAction.fulfilled, () => null), |
There was a problem hiding this comment.
This is a little counterintuitive for me at first glance. It seems like we'd want to set the new id as active. Seems like we could use a "why" comment here.
There was a problem hiding this comment.
I've added a comment to the code, this stems from the old activeAnnotationId being set. It is a little counter-intuitive here, but was causing a bit of a race condition for the scrolling behavior. This is something we can re-visit as well when we refine the selectedAnnotation flow if it doesn't fit.
There was a problem hiding this comment.
This flow seems really awkward. Can we allow box-annotations to set the id here rather than relying on the Preview SDK to do it?
src/components/AnnotationTarget/__tests__/AnnotationTarget-test.tsx
Outdated
Show resolved
Hide resolved
mickr
force-pushed
the
scroll-to-active-annotation
branch
from
c27db14 to
4d6beb6
Compare
* trigger scroll if needed when annotation becomes active. * set activeAnnotationId to null on creation to prevent holding onto old annotationId * Add test for isActive * Update reducer test
mickr
force-pushed
the
scroll-to-active-annotation
branch
from
4d6beb6 to
4d85b3f
Compare
src/components/AnnotationTarget/__tests__/AnnotationTarget-test.tsx
Outdated
Show resolved
Hide resolved
src/components/AnnotationTarget/__tests__/AnnotationTarget-test.tsx
Outdated
Show resolved
Hide resolved
src/components/AnnotationTarget/__tests__/AnnotationTarget-test.tsx
Outdated
Show resolved
Hide resolved
| builder.addCase(setActiveAnnotationIdAction, (state, { payload: annotationId }) => annotationId), | ||
| builder | ||
| .addCase(setActiveAnnotationIdAction, (state, { payload: annotationId }) => annotationId) | ||
| .addCase(createAnnotationAction.fulfilled, () => null), |
There was a problem hiding this comment.
This flow seems really awkward. Can we allow box-annotations to set the id here rather than relying on the Preview SDK to do it?
| const { annotationId, children, className, isActive, onSelect = noop, ...rest } = props; | ||
| const innerRef = React.useRef<HTMLAnchorElement>(null); | ||
|
|
||
| React.useImperativeHandle(ref, () => innerRef.current as HTMLAnchorElement, [innerRef]); |
There was a problem hiding this comment.
is the type cast here necessary?
There was a problem hiding this comment.
The correct type wasn't getting picked up so for this didn't figure it would be a big deal to typecast it.
| const getWrapper = (props = {}): ShallowWrapper => { | ||
| return shallow( | ||
| const getWrapper = (props = {}): ReactWrapper => { | ||
| return mount( |
There was a problem hiding this comment.
instead of a full mount, would a shallow combined with wrapper.find('a').dive() work?
There was a problem hiding this comment.
Shallow doesn't currently work with useEffect and throws hooks errors. I don't think it is a big enough reason to not just use mount.
* PR Feedback
* PR Feedback
3 participants
Uh oh!
There was an error while loading. Please reload this page.