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

Set default labels as index + 1 for canvas figures. #232

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

mathewjordan
Copy link
Member

@mathewjordan mathewjordan commented Nov 11, 2024

What does this do?

This sets the default label for canvas figures (thumbnails) in the media component in cases where the Canvas does not have a label. The label will be the (index + 1).toString() to accommodate all languages. This same label will be the alt of the rendered thumbnail as well. Filtering will also work in this cases, ex: "2" as the input value will be show the "2", "12", "21", "22", etc... thumbnails.

How to review?

Review with this fixture
https://samvera-labs.github.io/clover-iiif/docs/viewer/demo?iiif-content=https%3A%2F%2Fmarkpbaggett.github.io%2Fstatic_iiif%2Fmanifests%2Ftests%2Fservice_maps_0103.json

  • Canvas thumbnails will render as "1" and "2'.
  • Canvases are filterable

@mathewjordan mathewjordan self-assigned this Nov 11, 2024
Copy link
Member

@markpbaggett markpbaggett 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. I noticed one thing:

If you try search via filter, things work as expected except when the filter is applied the label of the Canvas always switches to 1 even if you search for 2 and it finds 2. Not sure you care about this, but I thought I'd mention. Looks good other than that. Thanks again.

@mathewjordan
Copy link
Member Author

Looks good. I noticed one thing:

If you try search via filter, things work as expected except when the filter is applied the label of the Canvas always switches to 1 even if you search for 2 and it finds 2. Not sure you care about this, but I thought I'd mention. Looks good other than that. Thanks again.

Oh, great catch. This update should catch that:

canvasIndex={mediaItems.findIndex((el) => el === item)}

Copy link
Member

@markpbaggett markpbaggett 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! :shipit:

@mathewjordan mathewjordan merged commit af38478 into main Nov 11, 2024
1 check passed
@mathewjordan mathewjordan deleted the allow-thumbnails-without-canvas-labels branch November 11, 2024 20:00
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