I'm separating this from the refactor theming patch into its own thread so we can hopefully reach agreement more quickly. You said the following in #19 in your comments:

+++ b/jw_player.module
@@ -565,163 +658,218 @@ function jw_player_preset_load($machine_name = NULL) {
+  // Load the JW Player library and supporting Drupal functions.
+  $element['#attached']['library'][] = array('jw_player', 'jwplayer');
+  $element['#attached']['library'][] = array('jw_player', 'jw_player_drupal');

libraries can have dependencies I think in Drupal 7 too. So jw_player_drupal could depend on jw_player and you don't have to specify it twice.

Yes they can, but here is my question. All the other components in $libraries['jwplayer'] are external. We have the player code (cloud or self hosted), JW Player key, and integration files.

The code in jw_player.drupal.js is the connection between JW Player and Drupal. Does it make sense to include it in the same grouping, or should it be its own item? I'm fine either way, but we were trying to make a more clear separation between the two.

Also had considered including jw_player.drupal.js in the 'integration files' in jw_player_libraries_info, but my preference is to have the player JS loaded as late as possible, not in the header. Let me know if you agree.

CommentFileSizeAuthor
#6 jw_player-add_player_js_file-2719977-6.patch1.7 KBsgdev
#6 interdiff-2719977-2-6.txt1.03 KBsgdev
#2 jw_player-add_player_js_file-2719977-2.patch1.56 KBsgdev

Comments

ron_s created an issue. See original summary.

sgdev’s picture

sgdev commented
StatusFileSize
new jw_player-add_player_js_file-2719977-2.patch1.56 KB

Here is a patch that only does two things: adds jw_player.drupal.js to the existing jwplayer library, and includes the JavaScript file. It won't do anything for now, because it's not #attached anywhere.

If we can agree on the basic approach for this, it's one less item we have to discuss relative to refactor theming. Thanks.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

All I meant here is to have the drupal library depend on the other one, so that is loaded automatically.

But anyway, IMHO we can't do this separately from the remaining feedback on that I added below because as I said, there, with what I suggested there, this dependency stuff is irrelevant anyway.

sgdev’s picture

sgdev commented
Status: Active » Needs review

Marking for review.

sgdev’s picture

sgdev commented

Yes, we're already aware of that, and needs to be addressed in the refactor theming patch. However I think this specific item can be handled on its own, separate from the rest of the theming issues.

All we're doing is adding a library and a JS file. They don't do anything at the moment, and won't until we have pre-render approach solidified anyway. Doesn't hurt anything.

sgdev’s picture

sgdev commented
StatusFileSize
new interdiff-2719977-2-6.txt1.03 KB
new jw_player-add_player_js_file-2719977-6.patch1.7 KB

Per comment #31 in refactor theming, there is an update needed to this patch. Please see attached, thanks.

sgdev’s picture

sgdev commented

We have a new update for this patch. We found that JW Player causes major issues when live preview is enabled in Views. Any view containing a player cannot be edited or saved. This is due to the JavaScript not being attached to the live preview.

I would post a patch for review, but we're stuck as before... until some of the other 7.x-2.x patches are committed, there are going to be code conflicts.

Making a note to post the patch once other things are committed. Otherwise not much I can do.

aitala’s picture

aitala commented

Any news on the fixed patch?

sgdev’s picture

sgdev commented

See my comment here: https://www.drupal.org/node/2713725#comment-12179489