-
Notifications
You must be signed in to change notification settings - Fork 29
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
lib-classifier: refactor single image viewer with VisXZoom #6390
base: master
Are you sure you want to change the base?
Changes from 33 commits
8a2889c
b274f8b
58d39be
878edd9
09d2d92
0d203b0
d598a4c
c93f219
39ad1a6
7b9da7a
3929816
b0ef4c6
f6b8415
f0d4086
75a568a
260d87b
467973b
066619a
49d6ca3
ec4651c
0c27c95
c1a434e
f2f2a56
8809d9f
f4bf4b0
88c669e
9c0a872
a52f9cd
39f75e4
c0937af
9c81f38
128c5b5
e8a4c45
c58b452
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,17 @@ | ||
import { render, screen } from '@testing-library/react' | ||
import { render, screen, waitFor } from '@testing-library/react' | ||
import { composeStory } from '@storybook/react' | ||
|
||
import Meta, { Default, mockTasks } from './CenteredLayout.stories.js' | ||
|
||
describe('Component > Layouts > Centered', function () { | ||
it('should render a subject and a task', function () { | ||
it('should render a subject and a task', async function () { | ||
const DefaultStory = composeStory(Default, Meta) | ||
render(<DefaultStory />) | ||
expect(screen.getByLabelText('Subject 1')).exists() // img aria-label from SVGImage | ||
expect(screen.getByText(mockTasks.init.strings.question)).exists() // task question paragraph | ||
|
||
// Mock the loading state transition | ||
Default.store.subjectViewer.onSubjectReady() | ||
|
||
await waitFor(() => expect(screen.getByLabelText('Subject 1')).exists()) // img aria-label from SVGImage | ||
await waitFor(() => expect(screen.getByText(mockTasks.init.strings.question)).exists()) // task question paragraph | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,18 @@ | ||
import { render, screen } from '@testing-library/react' | ||
import { render, screen, waitFor } from '@testing-library/react' | ||
import { composeStory } from '@storybook/react' | ||
|
||
import Meta, { Default, mockTasks } from './MaxWidth.stories.js' | ||
|
||
describe('Component > Layouts > MaxWidth', function () { | ||
|
||
it('should render a subject and a task', function () { | ||
it('should render a subject and a task', async function () { | ||
const DefaultStory = composeStory(Default, Meta) | ||
render(<DefaultStory />) | ||
expect(screen.getByLabelText('Subject 1')).exists() // img aria-label from SVGImage | ||
expect(screen.getByText(mockTasks.init.strings.question)).exists() // task question paragraph | ||
|
||
// Mock the loading state transition | ||
Default.store.subjectViewer.onSubjectReady() | ||
|
||
await waitFor(() => expect(screen.getByLabelText('Subject 1')).exists()) // img aria-label from SVGImage | ||
await waitFor(() => expect(screen.getByText(mockTasks.init.strings.question)).exists()) // task question paragraph | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,16 @@ | ||
import { render, screen } from '@testing-library/react' | ||
import { render, screen, waitFor } from '@testing-library/react' | ||
import { composeStory } from '@storybook/react' | ||
import Meta, { Default, mockTasks } from './NoMaxWidth.stories.js' | ||
|
||
describe('Component > Layouts > NoMaxWidth', function () { | ||
it('should render a subject and a task', function () { | ||
it('should render a subject and a task', async function () { | ||
const DefaultStory = composeStory(Default, Meta) | ||
render(<DefaultStory />) | ||
expect(screen.getByLabelText('Subject 1')).exists() // img aria-label from SVGImage | ||
expect(screen.getByText(mockTasks.init.strings.question)).exists() // task question paragraph | ||
|
||
// Mock the loading state transition | ||
Default.store.subjectViewer.onSubjectReady() | ||
|
||
await waitFor(() => expect(screen.getByLabelText('Subject 1')).exists()) // img aria-label from SVGImage | ||
await waitFor(() => expect(screen.getByText(mockTasks.init.strings.question)).exists()) // task question paragraph | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,12 @@ | ||
import { useEffect, useState } from 'react'; | ||
import { Box } from 'grommet' | ||
import PropTypes from 'prop-types' | ||
import { useEffect, useState } from 'react' | ||
|
||
import locationValidator from '../../helpers/locationValidator' | ||
import { useSubjectImage } from '@hooks' | ||
|
||
import SingleImageViewer from '../SingleImageViewer/SingleImageViewer.js' | ||
import SVGImage from '../SVGComponents/SVGImage' | ||
import SVGPanZoom from '../SVGComponents/SVGPanZoom' | ||
import locationValidator from '../../helpers/locationValidator' | ||
|
||
import SingleImageViewer from '../SingleImageViewer/SingleImageViewer' | ||
import FlipbookControls from './components' | ||
|
||
const DEFAULT_HANDLER = () => true | ||
|
@@ -19,38 +18,17 @@ const FlipbookViewer = ({ | |
flipbookAutoplay = false, | ||
invert = false, | ||
limitSubjectHeight = false, | ||
move, | ||
move = false, | ||
onError = DEFAULT_HANDLER, | ||
onKeyDown = DEFAULT_HANDLER, | ||
onReady = DEFAULT_HANDLER, | ||
playIterations, | ||
rotation, | ||
rotation = 0, | ||
setOnPan = DEFAULT_HANDLER, | ||
setOnZoom = DEFAULT_HANDLER, | ||
subject | ||
}) => { | ||
const [currentFrame, setCurrentFrame] = useState(defaultFrame) | ||
const [playing, setPlaying] = useState(false) | ||
const [dragMove, setDragMove] = useState() | ||
/** This initializes an image element from the subject's defaultFrame src url. | ||
* We do this so the SVGPanZoom has dimensions of the subject image. | ||
* We're assuming all frames in one subject have the same dimensions. */ | ||
const defaultFrameLocation = subject ? subject.locations[defaultFrame] : null | ||
const { img, error, loading, subjectImage } = useSubjectImage({ | ||
src: defaultFrameLocation.url, | ||
onReady, | ||
onError | ||
}) | ||
const { | ||
naturalHeight = 600, | ||
naturalWidth = 800 | ||
} = img | ||
|
||
const viewerLocation = subject?.locations ? subject.locations[currentFrame] : '' | ||
|
||
useEffect(() => { | ||
enableRotation() | ||
}, [img.src]) | ||
|
||
useEffect(() => { | ||
const reducedMotionQuery = window.matchMedia('(prefers-reduced-motion: reduce)') | ||
|
@@ -59,6 +37,18 @@ const FlipbookViewer = ({ | |
} | ||
}, []) | ||
|
||
const imageLocationUrl = subject?.locations[currentFrame]?.url | ||
|
||
const { img, error, loading, subjectImage } = useSubjectImage({ | ||
src: imageLocationUrl, | ||
onError, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing this from Regarding I noticed the metadata bug because earlier this year it was introduced by #5754 and fixed by #5952. On this branch, when I used my flipbook test project (listed in the FEM active projects for reference), this is what a submitted classification looks like when I flipped through 12 frames. Huge metadata causes classification exports to hang. Questions - Is the change from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think it's required. I'll try refactoring so |
||
onReady | ||
}) | ||
const { | ||
naturalHeight = 600, | ||
naturalWidth = 800 | ||
} = img | ||
|
||
const onPlayPause = () => { | ||
setPlaying(!playing) | ||
} | ||
|
@@ -67,57 +57,28 @@ const FlipbookViewer = ({ | |
if (event.key === ' ') { | ||
event.preventDefault() | ||
onPlayPause() | ||
} else { | ||
onKeyDown(event) | ||
} | ||
} | ||
|
||
const setOnDrag = (callback) => { | ||
setDragMove(() => callback) | ||
} | ||
|
||
const onDrag = (event, difference) => { | ||
dragMove?.(event, difference) | ||
} | ||
|
||
return ( | ||
<Box> | ||
<SVGPanZoom | ||
key={`${naturalWidth}-${naturalHeight}`} | ||
<SingleImageViewer | ||
enableInteractionLayer={enableInteractionLayer} | ||
enableRotation={enableRotation} | ||
frame={currentFrame} | ||
imgRef={subjectImage} | ||
invert={invert} | ||
limitSubjectHeight={limitSubjectHeight} | ||
maxZoom={5} | ||
minZoom={0.1} | ||
move={move} | ||
naturalHeight={naturalHeight} | ||
naturalWidth={naturalWidth} | ||
setOnDrag={setOnDrag} | ||
onKeyDown={handleSpaceBar} | ||
rotation={rotation} | ||
setOnPan={setOnPan} | ||
setOnZoom={setOnZoom} | ||
src={img.src} | ||
> | ||
<SingleImageViewer | ||
enableInteractionLayer={enableInteractionLayer} | ||
frame={currentFrame} | ||
height={naturalHeight} | ||
limitSubjectHeight={limitSubjectHeight} | ||
onKeyDown={handleSpaceBar} | ||
rotate={rotation} | ||
width={naturalWidth} | ||
> | ||
<g role='tabpanel' id='flipbook-tab-panel'> | ||
<SVGImage | ||
ref={subjectImage} | ||
invert={invert} | ||
move={move} | ||
naturalHeight={naturalHeight} | ||
naturalWidth={naturalWidth} | ||
onDrag={onDrag} | ||
src={viewerLocation.url} | ||
subjectID={subject.id} | ||
/> | ||
</g> | ||
</SingleImageViewer> | ||
</SVGPanZoom> | ||
subject={subject} | ||
/> | ||
<FlipbookControls | ||
currentFrame={currentFrame} | ||
locations={subject.locations} | ||
|
@@ -142,11 +103,13 @@ FlipbookViewer.propTypes = { | |
/** Passed from Subject Viewer Store */ | ||
invert: PropTypes.bool, | ||
/** Passed from Subject Viewer Store */ | ||
limit_subject_height: PropTypes.bool, | ||
limitSubjectHeight: PropTypes.bool, | ||
/** Passed from Subject Viewer Store */ | ||
move: PropTypes.bool, | ||
/** withKeyZoom() is for using keyboard pan and zoom controls while focused on the subject image */ | ||
onKeyDown: PropTypes.func, | ||
/** Passed from Subject Viewer Store */ | ||
onError: PropTypes.func, | ||
/** Passed from Subject Viewer Store */ | ||
onFrameChange: PropTypes.func, | ||
/** Passed from Subject Viewer Store and called when default frame's src is loaded */ | ||
onReady: PropTypes.func, | ||
/** Fetched from workflow configuration. Number preference for how many loops to play */ | ||
|
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.
Just out of curiosity is the 'todo' at the end in a Github Issue or referenced somewhere else in the code?
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 think #2110 is the related issue. I'm not sure the
NOTE
belongs in the README though. I could remove theNOTE
or update it to include the specific issue?