Support size=thumb parameter for embedded video files in Remarkup
Needs ReviewPublic
Actions

Authored by aklapper on Thu, Jun 12, 12:28.

Details

Reviewers

mainframe98

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T16042: Embedded videos are disproportionately huge

Summary

When embedding a video in a task description via Remarkup's {F1} it takes up to 95% of the width which is often unwanted.
Should support the same {F1,size=thumb} parameter which embedded image files already support.

In case of image files the resulting width for TRANSFORM_PREVIEW is hardcoded to 220px in PhabricatorFileThumbnailTransform.php::generateTransforms().
That feels subjectively small for video thumbs so arbitrarily bikeshed to hardcoded 400px instead.

Closes T16042

Test Plan

Upload a video file; put {F1,size=thumb} and {F1} into a task comment; resize viewport width.

Diff Detail

Repository

rP Phorge

Branch

T16042embedVideoWidth (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 2095
Build 2095: arc lint + arc unit

Event Timeline

aklapper created this revision.Thu, Jun 12, 12:28

Owners added a reviewer: O1: Blessed Committers.Thu, Jun 12, 12:28

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptThu, Jun 12, 12:28

aklapper requested review of this revision.Thu, Jun 12, 12:28

Harbormaster completed remote builds in B2092: Diff 3212.Thu, Jun 12, 12:28

Daimona awarded a token.Thu, Jun 12, 15:35

mainframe98 requested changes to this revision.Thu, Jun 12, 17:06

mainframe98 subscribed.

mainframe98 added inline comments.

src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
327	If `$options['size']` can be optional on line 315, then this line shouldn't access it unconditionally.

This revision now requires changes to proceed.Thu, Jun 12, 17:06

Uhm thanks! That was left from playing with the corresponding image code. As we already have a proper HTML tag class I'm not even sure what that size parameter is good for

Harbormaster completed remote builds in B2095: Diff 3215.Fri, Jun 13, 09:05

aklapper marked an inline comment as done.Fri, Jun 13, 09:08