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

fix(SUP-43106): bring captions in front of entry title/description #788

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Lkaltura
Copy link

@Lkaltura Lkaltura commented Jul 4, 2024

Description of the Changes

issue:
when playing an audio entry that includes captions, the captions are hidden behind the title and description of the entry.
fix:
bring captions in front of the title/description
Resolves: SUP-43106

@@ -44,6 +44,7 @@
}

.playkit-subtitles {
z-index: 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

how do subtitles look in non-audio entries after the change ? do they look ok ?

Copy link
Author

@Lkaltura Lkaltura Jul 7, 2024

Choose a reason for hiding this comment

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

in non-audio entries, you don't have title/description on the media itself, so it's not the same case.
however, I did encounter this issue in both types:
image
i noticed these buttons have z-index=1 as well as the title/description's = 1.
we shall set the captions at z-index > 1 so that it'll be in front of the title/descreption. but setting it to 2 would cover the buttons as seen in the above photo.
shall we increase the buttons with 1 so that we can set the caption to 2?
also, what should be taken into account to determine the value of caption-z-index, other than the buttons?

Copy link
Author

@Lkaltura Lkaltura Jul 7, 2024

Choose a reason for hiding this comment

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

this is how it would look like if :
.player .bottom-bar { z-index = 3 } (instead of 1 , 2 would also work fine)
and
.playkit-subtitles { z-index = 2 }
image

let me know if something else needs to be considered here

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, i agree with what sergey said below - we should localize the change to audio entries.
theres a variable on the global state called isAudio that should help with checking the entry type. see example here

@@ -44,6 +44,7 @@
}

.playkit-subtitles {
z-index: 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the changes should be applied only for audio-entries. Also, need check how it behaves with dual-screen plugin that manage caption z-index as well
https://github.com/kaltura/playkit-js-dual-screen/blob/e98dc19c8d19d8da36d5c0c9b2f07d29ad85612b/src/styles/global.scss#L1

Copy link
Author

@Lkaltura Lkaltura Jul 7, 2024

Choose a reason for hiding this comment

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

could you elaborate on the dual-screen ?
because there's no title/descreption on the media like audio entries,
i assume you're talking about the child video. what is the expected behavior with regards to the caption? it's supposed to be behind the captions still no?

Copy link
Author

Choose a reason for hiding this comment

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

either ways (z-index =1 or more) the captions would be in front of the 2 videos.

@@ -44,6 +44,7 @@
}

.playkit-subtitles {
z-index: 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, i agree with what sergey said below - we should localize the change to audio entries.
theres a variable on the global state called isAudio that should help with checking the entry type. see example here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants