feat: add canvas paintaings to media dialog#38
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds canvas painting images to the MediaDialog component and refactors the codebase to use the Traverse class for more efficient IIIF resource navigation.
- Implements IIIF Image Request URI utilities for dynamic image sizing
- Refactors MediaDialog to use Traverse class for finding canvas items
- Adds canvas paintings as selectable media alongside existing thumbnails and placeholders
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugin/utils/index.ts | Adds IIIF Image Request URI types and utility functions, updates getLabelByUserLanguage signature |
| src/plugin/Panel/index.tsx | Updates manifest label handling to be more defensive |
| src/plugin/Panel/index.test.tsx | Adds serialize mock for Canvas type in test setup |
| src/plugin/Panel/MediaDialog/style.module.css | Adds flex-wrap to content layout and removes commented code |
| src/plugin/Panel/MediaDialog/index.tsx | Major refactor to use Traverse class and add canvas paintings support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| width: resource?.width || 0, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
The JSDoc example has a malformed code block - it should end with three backticks instead of two.
| // @ts-expect-error - I don't want to go down the rabbit hole of type checking here | ||
| const label = getLabelByUserLanguage(painting?.label); |
There was a problem hiding this comment.
Using @ts-expect-error to bypass type checking makes the code fragile. Consider casting to the correct type or handling the type mismatch properly to maintain type safety.
| // @ts-expect-error - I don't want to go down the rabbit hole of type checking here | |
| const label = getLabelByUserLanguage(painting?.label); | |
| // Ensure painting.label is of the expected type for getLabelByUserLanguage | |
| const label = getLabelByUserLanguage( | |
| painting && painting.label | |
| ? (painting.label as Parameters<typeof getLabelByUserLanguage>[0]) | |
| : [] | |
| ); |
There was a problem hiding this comment.
Using array index as React key is an anti-pattern that can cause rendering issues. Use a unique, stable identifier like thumbnail-${thumbnail.id} instead.
The PR: