-
Notifications
You must be signed in to change notification settings - Fork 116
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
feat(viewer): Add support for disabling .boxdicom file type #1436
feat(viewer): Add support for disabling .boxdicom file type #1436
Conversation
* @inheritdoc | ||
*/ | ||
determineViewer(file, disabledViewers = [], viewerOptions = {}) { | ||
const isDicomFile = file.name === 'Dicom.boxdicom' || file.extension === 'boxdicom'; |
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 extension check is probably sufficient here
*/ | ||
determineViewer(file, disabledViewers = [], viewerOptions = {}) { | ||
const isDicomFile = file.name === 'Dicom.boxdicom' || file.extension === 'boxdicom'; | ||
const openWithAmbraEnabled = getProp(viewerOptions, 'IFrame.openWithAmbra'); |
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.
Instead of openWithAmbra, maybe the option should be disableDicom
focused because preview SDK knows nothing about an Ambra integration
determineViewer(file, disabledViewers = [], viewerOptions = {}) { | ||
const isDicomFile = file.name === 'Dicom.boxdicom' || file.extension === 'boxdicom'; | ||
const openWithAmbraEnabled = getProp(viewerOptions, 'IFrame.openWithAmbra'); | ||
// The IFrame viewer is disabled when the file is a Boxdicom file and Open_with_Ambra FF is enabled |
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.
We shouldn't mention FF from other codebases here
@@ -1,5 +1,6 @@ | |||
import IFrameViewer from '../IFrameViewer'; | |||
import BaseViewer from '../../BaseViewer'; | |||
import IFrameLoader from '../IFrameLoader'; |
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.
Sort the imports
expect(viewer).toBeUndefined(); | ||
}); | ||
|
||
test('should return a viewer if file is a boxnote and open_with_ambra FF is enabled', () => { |
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.
Could lump these together with a test.each
https://jestjs.io/docs/api#1-testeachtablename-fn-timeout
@JChan106, we'll probably want to go with |
@@ -1,3 +1,4 @@ | |||
import IFrameLoader from '../IFrameLoader'; |
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.
Can we move these tests IFrameLoader-test.js
, please?
}, | ||
}; | ||
const viewer = IFrameLoader.determineViewer(iframe.options.file, [], viewerOptions); | ||
if (viewerDefined) { |
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.
We typically want to avoid having logic and conditions in our tests. We could do something like this, though:
test.each`
disableDicom | fileType | viewerInstance
${false} | ${'boxdicom'} | ${IFrameViewer}
${true} | ${'boxdicom'} | ${undefined}
`
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.
Requesting changes to cancel the prior approval. Sorry for the confusion.
@@ -18,4 +31,24 @@ describe('lib/viewers/iframe/IFrameLoader', () => { | |||
EXT: ['boxnote', 'boxdicom'], | |||
}); | |||
}); | |||
|
|||
test.each([ | |||
// disableDicom, fileType, viewerInstance |
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.
In the future, consider using the tagged template literal syntax to incorporate these variable names directly into the inputs.
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.
Ohh, my bad I didn't know that actual syntax. Will update.
}, | ||
}, | ||
}); | ||
|
||
afterEach(() => { | ||
sandbox.verifyAndRestore(); |
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 isn't directly related to your change, but we should remove these when we edit a test file, if possible. It's not actually being used in any tests in this case, either.
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.
Maybe instead of Remove support...
in the PR title, it is Add support for disabling .boxdicom
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.
Looks great. Thanks for working through the feedback!
Removes preview support for boxdicom files. This is a prerequisite for modifying the default behavior to opening boxdicom files with Ambra.