Skip to content

[BUGFIX] Move fullscreen to own section #1531

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

Merged
merged 3 commits into from
Apr 25, 2025

Conversation

chrizzor
Copy link
Collaborator

PR to fix #1489
There are also changes in slub_digitalcollection and dfg-viewer.

@sebastian-meyer sebastian-meyer added the 🐛 bug A non-security related bug. label Mar 17, 2025
@sebastian-meyer sebastian-meyer changed the title #1489 Move fullscreen to own section [BUGFIX] Move fullscreen to own section Mar 17, 2025
Copy link
Member

@sebastian-meyer sebastian-meyer left a comment

Choose a reason for hiding this comment

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

I think simply moving the zoom and rotation buttons to their own section isn't sufficient. It still means that users of Kitodo.Presentation would have to add custom CSS in order to make sure, the zoom/rotation section is not visible in MultiView mode (which isn't even documented).

Instead, the template should only render the zoom/rotation section if the MultiView mode is not active. I suggest introducing a variable in the controller which can be used in the template to distinguish between MultiView and SingleDocument view and render the buttons accordingly.

@sebastian-meyer
Copy link
Member

Maybe we don't even need a new variable. Isn't there an URL parameter always set if MultiView is active?

@chrizzor
Copy link
Collaborator Author

Yes, there is the multiview parameter, which is also written to gp-multiview via Typoscript in the dfg-viewer extension:
https://github.com/slub/dfg-viewer/blob/471e1eec0e1fe67dc59197f8fa3ade9153243849/Configuration/TypoScript/Page/page.typoscript#L87

The parameter would then also have to be configured for Kitodo.presentation. Then it should also be available in the template, right?

@markusweigelt
Copy link
Contributor

I tested a version with your branches of all three extensions, and the buttons are still displayed.

What was the plan behind changing the fullscreen mode to a separate section without hiding the other sections?

The parameter would then also have to be configured for Kitodo.presentation. Then it should also be available in the template, right?

Yes and no. Currently, the condition to show gp-multiview is based on a parameter in Kitodo.Presentation, whereas in the DFG-Viewer extension, the condition is handled through docArray (see this line).

Perhaps we could unify these condition variants in the PageViewController (see here) and use a dedicated template variable from there. Maybe the existing multiview.

Is it actually correct to render the single view when docArray is empty? I think not. Instead of relying on a docArray check, we should use a dedicated template variable. If it’s empty, an empty grid should be displayed rather than defaulting to single view.

@chrizzor chrizzor force-pushed the 1489_multiview_hide_buttons branch from bcbbe52 to 292c886 Compare April 22, 2025 14:55
@chrizzor
Copy link
Collaborator Author

The MultiView parameter is now also available in the navigation plugin and is checked accordingly in the templates. The buttons should no longer be displayed in multi-view mode.

Is it actually correct to render the single view when docArray is empty? I think not. Instead of relying on a docArray check, we should use a dedicated template variable. If it’s empty, an empty grid should be displayed rather than defaulting to single view.

Does this mean that there are documents that neither contain pages nor reference documents?

@sebastian-meyer
Copy link
Member

There are no documents without pages or referenced documents, but there should be a way to enable an empty multiview grid (and then add documents manually). I am not sure if that's what @markusweigelt is refering to.

Copy link
Contributor

@markusweigelt markusweigelt left a comment

Choose a reason for hiding this comment

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

It's working on my end! I encountered an issue due to missing documents, but that has been resolved in this pull request: #1581
So if you’d like to retest, please merge the PR. The issue was caused by missing documents, which throws an exception before you can test the current PR.

@sebastian-meyer
Copy link
Member

Thanks for testing and the additional bugfix!

@sebastian-meyer sebastian-meyer merged commit 99e7442 into kitodo:main Apr 25, 2025
8 checks passed
Copy link

codecov bot commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (30eaa54) to head (f262a95).
Report is 1 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff      @@
##   main   #1531   +/-   ##
============================
============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug A non-security related bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Multi-View: General zoom, rotation and reset rotation buttons are not working
3 participants