feat(viewer): use unsupported file type error message for .boxdicom #1437
Conversation
| @@ -33,6 +33,12 @@ class IFrameLoader extends AssetLoader { | |||
| // The IFrame viewer is disabled when the file is a Boxdicom file and the disableDicom viewer option is enabled | |||
| if (disableDicom && isDicomFile) { | |||
| disabledViewers.push('IFrame'); | |||
There was a problem hiding this comment.
Do we still need this logic now that the extension is removed?
There was a problem hiding this comment.
Yeah I don't think we need it anymore, will remove.
| @@ -33,6 +33,12 @@ class IFrameLoader extends AssetLoader { | |||
| // The IFrame viewer is disabled when the file is a Boxdicom file and the disableDicom viewer option is enabled | |||
| if (disableDicom && isDicomFile) { | |||
| disabledViewers.push('IFrame'); | |||
|
|
|||
| // Removes boxdicom as a supported extension | |||
| const iframeViewer = this.viewers[0].EXT; | |||
There was a problem hiding this comment.
Can we simplify things a bit by breaking out the viewers into multiple entries? Maybe something like:
const VIEWERS = [
{
NAME: 'IFrame',
CONSTRUCTOR: IFrameViewer,
REP: ORIGINAL_REP_NAME,
EXT: ['boxdicom'],
},
{
NAME: 'IFrame',
CONSTRUCTOR: IFrameViewer,
REP: ORIGINAL_REP_NAME,
EXT: ['boxnote'],
},
];
...
if (disableDicom && isDicomFile) {
this.viewers = this.viewers.filter(viewer => !viewer.EXT.includes('boxdicom'));
}
| @@ -43,6 +51,7 @@ describe('lib/viewers/iframe/IFrameLoader', () => { | |||
| }; | |||
| const viewer = IFrameLoader.determineViewer(iframe.options.file, [], viewerOptions); | |||
| expect(viewer).toEqual(viewerInstance); | |||
| expect(IFrameLoader.getViewers().some(v => v.EXT.includes('boxdicom'))).toEqual(!disableDicom); | |||
There was a problem hiding this comment.
Do we need this assertion if we already have the one above it?
There was a problem hiding this comment.
We could, the assertion checks if the list of viewers is correct based on the disableDicom viewer option. While the above assertion checks if the viewer instance is correct.
| @@ -30,9 +36,9 @@ class IFrameLoader extends AssetLoader { | |||
| determineViewer(file, disabledViewers = [], viewerOptions = {}) { | |||
| const isDicomFile = file.extension === 'boxdicom'; | |||
| const disableDicom = getProp(viewerOptions, 'IFrame.disableDicom'); | |||
| // The IFrame viewer is disabled when the file is a Boxdicom file and the disableDicom viewer option is enabled | |||
| // Removes boxdicom as a supported extension when the file is a Boxdicom file and the disableDicom viewer option is enabled | |||
There was a problem hiding this comment.
Do we even need to check if isDicomFile is true anymore? If disableDicom option is passed in, would it work if we just remove it from the this.viewers?
There was a problem hiding this comment.
I think you're right, it doesn't really matter about the current file, we should just remove the dicom viewer from the supported viewers if the disableDicom is true.
There was a problem hiding this comment.
May want to update the comment as well to be consistent with the code
| @@ -30,9 +36,9 @@ class IFrameLoader extends AssetLoader { | |||
| determineViewer(file, disabledViewers = [], viewerOptions = {}) { | |||
| const isDicomFile = file.extension === 'boxdicom'; | |||
| const disableDicom = getProp(viewerOptions, 'IFrame.disableDicom'); | |||
| // The IFrame viewer is disabled when the file is a Boxdicom file and the disableDicom viewer option is enabled | |||
| // Removes boxdicom as a supported extension when the file is a Boxdicom file and the disableDicom viewer option is enabled | |||
There was a problem hiding this comment.
May want to update the comment as well to be consistent with the code
Uses the unsupported file type error message instead of the account error message for Box Dicom files.