Skip to content

fix: Removed assumption of annotation SVG's existence [DC-498]#50

Merged
Jarbuckle merged 4 commits intomainfrom
joel/dc-498-dzi-viewer-annotation-issue
Jan 31, 2025
Merged

fix: Removed assumption of annotation SVG's existence [DC-498]#50
Jarbuckle merged 4 commits intomainfrom
joel/dc-498-dzi-viewer-annotation-issue

Conversation

@Jarbuckle
Copy link
Contributor

@Jarbuckle Jarbuckle commented Jan 30, 2025

fix: Removed assumption of annotation SVG's existence [DC-498]

What

DZI images without associated annotation SVGs were failing to render. Since it is valid for DZI images to not have associated annotation SVGs, removed this assumption to prevent issues during rendering.

How

  • Replaced compose (which was always created via buildCompositor and assumed an SVG's existence) with a variable render that falls back to putImageData when no SVG is found.

PR Checklist

  • Is your PR title following our conventional commit naming recommendations?
  • Have you filled in the PR Description Template?
  • Is your branch up to date with the latest in main?
  • Do the CI checks pass successfully?
  • Have you smoke tested the example applications?
  • Did you check that the changes meet accessibility standards?
  • Have you tested the application on these browsers?
    • Chrome (Fully supported)
    • Firefox (Major bug fixes supported)
    • Safari (Major bug fixes supported)

@Jarbuckle Jarbuckle requested a review from a team as a code owner January 30, 2025 17:57
@Jarbuckle Jarbuckle changed the title Removed the assumption that annotation SVG exists fix Removed the assumption that annotation SVG exists [DC-498] Jan 30, 2025
@Jarbuckle Jarbuckle changed the title fix Removed the assumption that annotation SVG exists [DC-498] fix: Removed the assumption that annotation SVG exists [DC-498] Jan 30, 2025
@Jarbuckle Jarbuckle changed the title fix: Removed the assumption that annotation SVG exists [DC-498] fix: Removed assumption of annotation SVG's existence [DC-498] Jan 30, 2025
Copy link
Collaborator

@lanesawyer lanesawyer left a comment

Choose a reason for hiding this comment

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

This fixes the issue, but I think the image check logic should live in buildCompositor instead so that we don't have two ctx.putImageData(image, 0, 0) lines floating around.

Not completely opposed to shipping it as-is though. The logic here isn't too complex. Just nervous that it'll grow as we polish up the viewer and then we'll be duplicating more and more instead of having a single function that does the proper drawings.

@Jarbuckle Jarbuckle closed this Jan 31, 2025
@Jarbuckle Jarbuckle reopened this Jan 31, 2025
@Jarbuckle
Copy link
Contributor Author

I've made updates in the bkp-client PR first, @lanesawyer -- could you take a look over there? If it's good there, I'll make updates on this side.

Copy link
Collaborator

@lanesawyer lanesawyer 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 after merge!

@Jarbuckle Jarbuckle merged commit fe2d44c into main Jan 31, 2025
5 checks passed
@Jarbuckle Jarbuckle deleted the joel/dc-498-dzi-viewer-annotation-issue branch January 31, 2025 23:27
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