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

Media viewer controls rendered behind DSpace header #2594

Merged
merged 3 commits into from Nov 9, 2023

Conversation

davide-negretti
Copy link
Contributor

@davide-negretti davide-negretti commented Oct 31, 2023

References

Description

The ds-header-navbar-wrapper component covers the media player control buttons because it had an higher z-index than its sibling <main> element, which contains the ngx-gallery-preview component.

Note that the media preview component itself has the highest z-index, but only the z-index of <main> should be taken into account.

In this PR I removed the z-index of ds-header-navbar-wrapper and I added the same z-index to the ds-navbar component, which is only shown when the collapsible navbar is expanded on mobile screens.

Instructions for Reviewers

Ensure that the media viewer is enabled:

mediaViewer:
  image: true
  video: true

Open any item with an image, e.g. https://sandbox.dspace.org/entities/publication/ffc161e0-1bd9-415c-a479-ae48b63c43e8

  • On desktop and mobile: click on the image to open its preview, then ensure that the header doesn't cover the control buttons of the viewer
  • On mobile: ensure that the collapsible navbar works fine in all pages containing elements with a z-index

Screenshots

Schermata del 2023-10-31 15-19-05

Schermata del 2023-10-31 16-06-32

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@tdonohue tdonohue added bug usability high priority 1 APPROVAL pull request only requires a single approval to merge component: Item (Archived) Item display or editing port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release labels Oct 31, 2023
@tdonohue tdonohue mentioned this pull request Nov 3, 2023
8 tasks
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@davide-negretti : I gave this a test today. It works perfectly for the dspace theme. But, if I instead change to the custom theme then I still see the bug. Here's a screenshot showing that the header still hides the media viewer controls in the custom theme.

custom-theme

So, if there's a way to also fix it for the custom theme, that'd be best. Maybe some of the dspace theme specific changes in this PR need to be made globally?

@davide-negretti davide-negretti force-pushed the DURACOM-180 branch 2 times, most recently from f73c616 to 814493e Compare November 9, 2023 14:24
@tdonohue tdonohue self-requested a review November 9, 2023 15:31
@davide-negretti
Copy link
Contributor Author

@tdonohue I'm fixing custom theme and base theme as well

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @davide-negretti ! This now fixes the bug in both the dspace and custom themes

@tdonohue tdonohue merged commit 684846a into DSpace:main Nov 9, 2023
11 checks passed
@dspace-bot
Copy link

Successfully created backport PR for dspace-7_x:

@tdonohue tdonohue added this to the 8.0 milestone Nov 9, 2023
@tdonohue tdonohue removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Nov 9, 2023
@tdonohue tdonohue mentioned this pull request Nov 9, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge bug component: Item (Archived) Item display or editing high priority usability
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Media viewer controls rendered behind DSpace header
3 participants