[MAINTAIN-151] added frameborder to embedded videos in all Open Y themes #2526
Conversation
|
Can one of the admins verify this patch? Use "o+k to test" or ''t+est this please" for manual build execution. |
|
ok to test |
Build comment file:Check Open Y Installation Wizard at http://install.openy.cibox.tools/build3308/install.php Test PASSed. |
|
Open Y Upgrade Path site check installed at http://upgrade.openy.cibox.tools/build3308/ |
| function openy_carnation_preprocess_video_embed_iframe(array &$variables) { | ||
| if ($attr = $variables['attributes']) { | ||
| /** @var \Drupal\Core\Template\Attribute $attr */ | ||
| $attr->setAttribute('frameborder', 1); |
There was a problem hiding this comment.
what if there is already frameborder, but not 1. It'll be overridden by this code.
Let's be less generic
If no frameborder - set it to 1
if frameborder = 0, or any other number - skip it
There was a problem hiding this comment.
There was a problem hiding this comment.
@aleevas as a content manager I can set frameborder explicitly to 0. your code would break it and reset to 1
We need to check if frameborder is set to any value before setting it to 1 in our case.
If it is set to any value - we must avoid of setting to 1
There was a problem hiding this comment.
@podarok
Sorry, but I can't find any ability for content manager to change value for farmeborder attribute.
As you can see on screenshot above, we have settings only for the width and the height attribute, the frameborder attribute has a hardcoded value = 0. And this is actual in any other places where mentioning the framborder attribute even in others modules.
Why we expect the border=1 as it written in jira issue?
Please help me to understand what I missed here
cc @hamrant
There was a problem hiding this comment.
No need to underestimate content managers
They can edit HTML in WYSIWYG in order to set 0 for frameborder. @aleevas
… in all Open Y themes
Build comment file:Check Open Y Installation Wizard at http://install.openy.cibox.tools/build3309/install.php Test PASSed. |
|
Open Y Upgrade Path site check installed at http://upgrade.openy.cibox.tools/build3309/ |
|
|
| if (!$attr || !$attr instanceof Attribute) { | ||
| return; | ||
| } | ||
| if (!$attr->hasAttribute('frameborder')) { |
There was a problem hiding this comment.
It looks like this condition not works... There is no border on the http://upgrade.openy.cibox.tools/build3309/about.
So on this preprocess level, we always have value for frameborder.
frameborderattribute hardcoded in therenderEmbedCodemethod:
video_embed_field/src/Plugin/video_embed_field/Provider/Vimeo.phpvideo_embed_field/src/Plugin/video_embed_field/Provider/YouTube.phpvideo_embed_field/src/Plugin/video_embed_field/Provider/YouTubePlaylist.php
- Element defined in the
video_embed_field/src/Element/VideoEmbedIFrame.phpwe have theregetInfo()method and'#pre_render'withpreRenderInlineFrameEmbedfunction.
Proposed solution: add a custom method to the '#pre_render' array after preRenderInlineFrameEmbed function and set frameborder attribute to 1 inside this function. Element info can be altered with hook_element_info_alter, see core/lib/Drupal/Core/Render/ElementInfoManager.php:
// Allow modules to alter the element type defaults.
$this->moduleHandler->alter('element_info', $info);
$this->themeManager->alter('element_info', $info);In this case, frameborder can be changed later in preprocess functions or in a different place.
I think we can move this to https://github.com/open-y-subprojects/openy_features/tree/main/openy_media/modules/openy_media_video.
There was a problem hiding this comment.
@hamrant thank you for your time and great advise.
I've made a PR to openy-features github open-y-subprojects/openy_features#8
I think this PR we can close
cc @podarok
There was a problem hiding this comment.
@podarok this PR looks ok for me - open-y-subprojects/openy_features#8
Feel free to merge and release a new version of openy_features.
|
@aleevas https://github.com/open-y-subprojects/openy_features/releases/tag/1.5 Feel free to create PR to current repo with this release as a baseline in composer.json |
|
Fixed in #2529 |
Original Issue, this PR is going to fix: MAINTAIN-151
The fix should add the border to all embedded video entities
Steps for review
log as admin
go to edit node (blog post foe example)
to add a new External video entity
save node and verify that iframe has border=1
verify that for all themes
General checks
Thank you for your contribution!