Skip to content

[FINNA-3012] Renormalize displaying videos#3155

Draft
LuomaJuha wants to merge 31 commits into
NatLibFi:devfrom
LuomaJuha:fix-finna-video-js
Draft

[FINNA-3012] Renormalize displaying videos#3155
LuomaJuha wants to merge 31 commits into
NatLibFi:devfrom
LuomaJuha:fix-finna-video-js

Conversation

@LuomaJuha
Copy link
Copy Markdown

@LuomaJuha LuomaJuha commented Feb 17, 2025

  • Removes 2 custom elements, adds a component like others

  • Removes displaying of videos inside popups, instead opens them in their respective windows

  • Fixes displaying if inline video is set to true

  • Use Lightbox instead of finna-popup

  • Checked that templates record-video-player and record-online-urls were modified in kavis templates. They have a locally managed player code.

#3307 Requires

@LuomaJuha LuomaJuha requested a review from EreMaijala February 17, 2025 16:02
@LuomaJuha
Copy link
Copy Markdown
Author

LuomaJuha commented Feb 18, 2025

Actually managed to break this somehow after yesterday... Fixing... Edit: fixed

@LuomaJuha LuomaJuha removed the request for review from EreMaijala February 18, 2025 14:44
@EreMaijala
Copy link
Copy Markdown

@LuomaJuha Voitko mergetä uusimman dev:n?

@LuomaJuha
Copy link
Copy Markdown
Author

@EreMaijala dodi, nyt pitäis olla

Comment thread themes/finna2/js/finna-video-player.js Outdated
Comment thread themes/finna2/js/finna-video-player.js Outdated
Comment thread themes/finna2/js/finna-video-player.js Outdated
Comment thread themes/finna2/js/finna-video-player.js Outdated
Comment thread themes/finna2/js/finna-video-player.js Outdated
// Template for using proper styles, when appending video viewer inside an element.
%iframe-centering-template {
height: $inline-video-player-desktop-height;
@media (max-width: $screen-md-max) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Käytä Bootstrapin media-helpereitä.

@@ -0,0 +1,57 @@
<?php
/**
* Component for opening or displaying proper video content.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
* Component for opening or displaying proper video content.
* Component for opening or displaying video content.

* @param string description Text description inside the button. Default is format_Video translated.
*/

$inlineVideoPlayer = $this->inline ?? false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missä dokumentaatio inline-parametrille?

];

if ($index === 0 && $inlineVideoPlayer) {
$buttonAttributes['class'] .= ' active-video';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Aloittaako tämä toiston automaattisesti? Jos aloittaa, niin miksi? Jos ei, niin mikä tämän merkitys on?

?>
<button <?=$this->htmlAttributes($buttonAttributes);?> <?php if ($videoSources): ?>data-video-sources="<?=htmlspecialchars(json_encode($videoSources), ENT_QUOTES, 'UTF-8');?>"<?php endif;?>>
<?=$this->icon('video-play', 'video-play-icon');?>
<span class="video-desc"><?=$this->escapeHtml($this->truncate(ucfirst($description), 30))?></span>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Kuuluuko ucfirst tänne? Vai olisiko se ennemmin kutsujan tai driverin tms. asia?

LuomaJuha and others added 9 commits October 8, 2025 10:43
Co-authored-by: Ere Maijala <ere.maijala@helsinki.fi>
Co-authored-by: Ere Maijala <ere.maijala@helsinki.fi>
Co-authored-by: Ere Maijala <ere.maijala@helsinki.fi>
Co-authored-by: Ere Maijala <ere.maijala@helsinki.fi>
Co-authored-by: Ere Maijala <ere.maijala@helsinki.fi>
Co-authored-by: Ere Maijala <ere.maijala@helsinki.fi>
@LuomaJuha LuomaJuha marked this pull request as draft May 26, 2026 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants