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

Serialize master file display title to iiif ranges #5556

Merged
merged 4 commits into from
Jan 10, 2024

Conversation

masaball
Copy link
Contributor

Closes #5539

@masaball masaball marked this pull request as draft January 10, 2024 15:01
cjcolvar
cjcolvar previously approved these changes Jan 10, 2024
Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

This looks fine to me.

I was a little concerned about changing the value of label in stream_details but a quick look through the code didn't reveal any place that was using it besides IiifCanasPresenter. (I did see that

<a class='video-link' id='video-link-<%= @master_file.id %>' href="<%= master_file_url(@master_file.id) %>" target="_blank" rel="noreferrer noopener"><%= @master_file.title || @master_file.media_object.title %></a>
isn't using it or embed_title but that's out of scope of this PR.)

I'm also wandering if having a nil label in the manifest will cause any problems for ramp. I thought we may have been including them in other metadata but can't remember. I don't think that should hold up this PR though if you think it is okay.

@masaball
Copy link
Contributor Author

masaball commented Jan 10, 2024

When I was working on this yesterday the changes worked with existing items. However, I just uploaded a new item in this branch to check the error for Ramp #334 and the section label is broken so this will need to be reworked. It looks like there is different behavior for multi-section vs single section item title generation that I missed.

@masaball masaball force-pushed the manifest_item_label branch from 515557b to 9222907 Compare January 10, 2024 16:37
@masaball masaball marked this pull request as ready for review January 10, 2024 18:20
@masaball masaball merged commit 8242230 into develop Jan 10, 2024
@masaball masaball deleted the manifest_item_label branch January 10, 2024 18:54
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