Problem/Motivation

Compare with 7.x-2.x, implement the same logic.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 interdiff-2713925-5-9.txt10.7 KBjohnchque
#9 add_missing_stretch_and-2713925-9.patch12.24 KBjohnchque
#5 interdiff-2713925-3-5.txt1.2 KBjohnchque
#5 add_missing_stretch_and-2713925-5.patch9.26 KBjohnchque
#3 add_missing_stretch_and-2713925-3.patch9.28 KBjohnchque
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Assigned: Unassigned » johnchque
johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Status: Active » Needs review
FileSize
add_missing_stretch_and-2713925-3.patch9.28 KB

Patch added. :)

Arla’s picture

Arla
they/them
CreditAttribution: Arla at MD Systems GmbH commented

Based on a very quick patch read-through (will apply and test asap):

  1. +++ b/config/schema/jw_player.schema.yml
@@ -21,12 +21,21 @@ jw_player.preset.*:
+        stretching:
+          type: label
+          label: 'Stretching'
+        responsive:
+          type: label
+          label: 'Responsive'
+        aspectratio:
+          type: label
+          label: 'Aspect Ratio'

    Looks like stretching and aspectration should be "type: string" (label is for translatable/human-readable values), and responsive should be boolean. 

  2. +++ b/src/Entity/Jw_player.php
@@ -71,6 +71,13 @@ class Jw_player extends ConfigEntityBase implements Jw_playerInterface {
+      $this->settings['width'] = $this->settings['width'] . '%';

    .=

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
FileSize
add_missing_stretch_and-2713925-5.patch9.26 KB
interdiff-2713925-3-5.txt1.2 KB

Totally true. :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work
Issue tags: +Needs tests 
  1. +++ b/js/jw_player.preset.js
@@ -0,0 +1,23 @@
+          $(width_suffix).text('pixels');

    This should use Drupal.t() 

  2. +++ b/src/Form/JwplayerPresetAdd.php
@@ -258,8 +305,56 @@ class JwplayerPresetAdd extends EntityForm {
+      if (empty($preset->settings['width']) || !is_numeric($preset->settings['width'])) {
+        $form_state->setErrorByName('settings][width', t('Width field is required and must be a numeric value.'));

    We can use a #type integer field in 8.x to at least do the numeric validation automatically for us.

    Same for the > 0.

    required might eventually be dynamic, so that part not.

    Should also use $this->t()

Tests would also be great. but I think we are once more blocked on using the cloud player and it is currently not yet possible to set settings then. One of the recent 7.x patches added support for that, so we can finally do that there then.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Issue tags: -Needs tests
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

#2716735: Validation additions, .install update, and minor cleanups might also have some fixes for this.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
add_missing_stretch_and-2713925-9.patch12.24 KB
interdiff-2713925-5-9.txt10.7 KB

Changes made based on discussions with @Berdir and comments above. :)

  • Berdir committed 8566b7e on 8.x-1.x authored by yongt9412 
    Issue #2713925 by yongt9412: Add missing stretch and responsive design...
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Fixed

Looks good, nice.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.