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.

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

fix: Load the palette color from a promise #434

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wayfarer3130
Copy link
Contributor

The palette colour data can come back as a promise, so that it is lazy loaded as required.
The conversion from palette colour is fast, but there does seem to be a rerender somewhere that is relatively slow in the newer version - this doesn't seem to be related to the changes here as I can revert them and it is still slow, but version 4.0.4 was faster.

Changed to have the image loader use async instead of promises to deal with a deep nesting of things.

Note there is a performance issue in this build - it wasn't introduced
by these changes, but it 5-10 times slower than earlier versions (not
sure how early to go to have it fast - was using 4.0.4
@netlify
Copy link

netlify bot commented Mar 7, 2022

✔️ Deploy Preview for cornerstone-wado-image-loader ready!

🔨 Explore the source changes: 8ba0b46

🔍 Inspect the deploy log: https://app.netlify.com/sites/cornerstone-wado-image-loader/deploys/62263b04205b730007e78699

😎 Browse the preview: https://deploy-preview-434--cornerstone-wado-image-loader.netlify.app

@sedghi sedghi self-requested a review August 3, 2022 11:52
@sedghi sedghi changed the title fix(cornerstoneWadoImageLoader):Load the palette color from a promise fix: Load the palette color from a promise Aug 3, 2022
@@ -7,6 +7,8 @@ import getMinMax from '../shared/getMinMax.js';
import isJPEGBaseline8BitColor from './isJPEGBaseline8BitColor.js';
import { getOptions } from './internal/options.js';

/*eslint complexity: ["error", 37]*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove this?

Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments needs to be resolved first, Looks good otherwise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants