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
Mirador Test Previewer #2550
base: master
Are you sure you want to change the base?
Mirador Test Previewer #2550
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it a deliberate choice to add the Mirador previewer here, and not in invenio-previewer
? Is it because it requires the IIIF endpoints?
I added a few comments
@@ -10,7 +10,7 @@ | |||
#} | |||
|
|||
|
|||
{%- macro preview_file(preview_endpoint, pid_value, filename, is_preview, include_deleted, id='preview-iframe', width='100%', height='400' ) %} | |||
{%- macro preview_file(preview_endpoint, pid_value, filename, is_preview, include_deleted, id='preview-iframe', width='100%', height='800' ) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are changing the default height
for all previewers here, not only Mirador. Is this what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
400px is pretty small, so making it bigger for everyone might be something desirable. See e.g. https://new-cds.cern.ch/records/fp18d-jc149:
We have to be a bit careful on mobile though, it's a bit difficult to touch/scroll at the right place to get past the previewer:
We should probably rewrite to use CSS classes for styling, with 80vh
or similar (that way anyone can override also on their instance).
invenio_app_rdm/records_ui/templates/semantic-ui/invenio_app_rdm/records/mirador_preview.html
Outdated
Show resolved
Hide resolved
invenio_app_rdm/records_ui/templates/semantic-ui/invenio_app_rdm/records/mirador_preview.html
Outdated
Show resolved
Hide resolved
invenio_app_rdm/records_ui/templates/semantic-ui/invenio_app_rdm/records/mirador_preview.html
Outdated
Show resolved
Hide resolved
invenio_app_rdm/records_ui/templates/semantic-ui/invenio_app_rdm/records/mirador_preview.html
Outdated
Show resolved
Hide resolved
This PR was automatically marked as stale. |
85463eb
to
26e66cd
Compare
01c5689
to
8900578
Compare
0077dfc
to
a632fb6
Compare
a25f1c2
to
d9829b2
Compare
❤️ Thank you for your contribution!
Description
Testing integration of Mirador Previewer
Closes Issue zenodo/zenodo-rdm#819
Depends on: #2652
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Third-party code
If you've added third-party code (copy/pasted or new dependencies), please reach out to an architect.
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: