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

OHIF-2612: Fix ImageThumbnail unnecessary renderToCanvas calls #2613

Open
wants to merge 2 commits into
base: v2-legacy
Choose a base branch
from

Conversation

ngutman
Copy link

@ngutman ngutman commented Nov 17, 2021

@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #2613 (d461f7e) into master (1e72684) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2613      +/-   ##
==========================================
- Coverage   13.20%   13.14%   -0.06%     
==========================================
  Files         289      289              
  Lines        7816     7849      +33     
  Branches     1510     1512       +2     
==========================================
  Hits         1032     1032              
- Misses       5474     5505      +31     
- Partials     1310     1312       +2     
Impacted Files Coverage Δ
platform/core/src/classes/StudyPrefetcher.js 0.69% <0.00%> (-0.03%) ⬇️
...latform/core/src/classes/metadata/StudyMetadata.js 1.29% <0.00%> (-0.02%) ⬇️
.../src/studies/services/wado/studyInstanceHelpers.js 2.04% <0.00%> (-0.19%) ⬇️
...form/core/src/utils/isDisplaySetReconstructable.js 3.37% <0.00%> (ø)
...r/src/connectedComponents/ConnectedStudyBrowser.js 0.00% <0.00%> (ø)
platform/viewer/src/connectedComponents/Viewer.js 0.00% <0.00%> (ø)
...tform/viewer/src/connectedComponents/ViewerMain.js 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79548d2...d461f7e. Read the comment docs.

@@ -86,7 +86,7 @@ function ImageThumbnail(props) {
}, [purgeCancelablePromise]);

useEffect(() => {
if (image.imageId) {
if (image.imageId && isLoading) {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we add isLoading in the dependency array of the useEffect too?

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.

Looks good to me, please see my comment

@stale
Copy link

stale bot commented Jul 10, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale 🥖 label Jul 10, 2022
@sedghi sedghi removed the Stale 🥖 label Jan 11, 2023
@sedghi sedghi changed the base branch from master to v2-legacy June 19, 2023 13:20
@sedghi
Copy link
Member

sedghi commented Jun 19, 2023

base has changed, read more here #3477

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

Successfully merging this pull request may close these issues.

2 participants