Problem/Motivation

stuff gets weird when the file is translatable but the alt and title are not. In practice this does not make sense anyway. During testing, the result is that changing an image means deleting it, and the alt and title are deleted when that happens also, so their values are not sync'd. since that behavior is image specific.

Proposed resolution

Add some JS to make the dependent groups always checked (and readonly) when the master group is checked.
When check "File", "Alt" and "Title" should be automatically checked and cannot be unchecked.

Remaining tasks

  • Create a patch
  • Review the patch

User interface changes

TBD

API changes

TBD

Original report by YesCT

#1807692-120: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget 4. @YesCT

4. things make sense when file, alt, title are all translatable.
they make sense when file is not but alt and title are translatable.
stuff gets weird when the file is translatable but the alt and title are not. In practice this does not make sense anyway. During testing, the result is that changing an image means deleting it, and the alt and title are deleted when that happens also, so their values are not sync'd. since that behavior is image specific. it's a works as designed or wont fix too. since the code is generic, I think its behavior is ok.

#121 @plach

Yep, this is why I originally went for a less flexible approach: less stuff to figure out for the user. Also here we might have a follow-up and add some JS to make the dependent groups always checked (and readonly) when the master group is checked. E.g. when I check "File", "Alt" and "Title" are automatically checked and cannot be unchecked.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexh58’s picture

Hi there. I saw this was tagged with Novice, and since I never submitted a core patch before, this looked like an easy one.

I think I interpreted the issue correctly, and the attached patch I've tested. But it definitely needs review and more testing I'm sure.

Status: Needs review » Needs work
YesCT’s picture

FileSize
277.74 KB
68.9 KB
285.61 KB

functional review (done easily with the dreditor simplytest.me button):

on the content type manage field edit tab for an image:
it's nice!

default.png

greyd_out.png

please make it also do that on the overall content language settings page (under Configuration)

content_language_settings.png

YesCT’s picture

Thanks! I would not have known where to start to implement that js!

a standards review.

+++ b/core/modules/translation_entity/translation_entity.admin.jsundefined
@@ -3,6 +3,26 @@
+ * Forces all options to be marked as translatable if the file itself is translatable.

make the one line summary limit of 80 chars.
http://drupal.org/node/172169 says to follow http://drupal.org/node/1354#drupal

All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.). They must provide a brief description of what a function does, what a class does, what a file contains, etc.

+++ b/core/modules/translation_entity/translation_entity.admin.jsundefined
@@ -3,6 +3,26 @@
+    // Watch changes on all of these checkboxes

inline should also be sentence that ends in a period.

+++ b/core/modules/translation_entity/translation_entity.admin.jsundefined
@@ -3,6 +3,26 @@
+	$checkboxes.prop('checked', true).not($(this)).prop('disabled', true);
+      } else {
+	$checkboxes.prop('disabled', false);

use spaces to indent instead of tabs.

http://drupal.org/coding-standards#indenting

+++ b/core/modules/translation_entity/translation_entity.admin.jsundefined
@@ -3,6 +3,26 @@
+
+

use just one newline to separate things.

saltednut’s picture

+++ b/core/modules/translation_entity/translation_entity.admin.jsundefined
@@ -3,6 +3,26 @@
+ * Forces all options to be marked as translatable if the file itself is translatable.
+ */
+Drupal.behaviors.translationEntityFileOptions = {
+  attach: function (context) {
+    var $collection = $(context).find('.form-item-instance-settings-translation-sync');
+    var $checkboxes = $collection.find(':input');
+
+    // Watch changes on all of these checkboxes
+    $checkboxes.change(function() {
+      if ($collection.find('input[value="file"]').is(':checked')) {
+	$checkboxes.prop('checked', true).not($(this)).prop('disabled', true);
+      } else {
+	$checkboxes.prop('disabled', false);
+      }
+    });
+  }
+};

Should this be more generic if there are other places (besides File fields) where dependents need to be checked?

alexh58’s picture

Status: Needs work » Needs review
FileSize
5.04 KB

Awesome. Thank you for the review and resources. I had no idea about simpletest.me and the dreditor plugin.

And @brantwynn, that was my thought too when going at this the first time. So this patch takes a more generic approach and uses a JS settings property on the element, so if a module needed to extend or add to that array of selectors, they should be able to do so easily.

And I'm hoping I didn't reinvent the wheel with how I handled that table, which now has the same functionality of dependent columns in a translatable field. I don't believe I've ever seen this, even in a contrib module, so I came up with an approach that solves this problem and works for the other standalone field page.

Please review!

YesCT’s picture

just re-wrapped the comments to be 80 chars.

manual testing next.

YesCT’s picture

FileSize
255.82 KB
216.08 KB
204.73 KB

is there another field I could check that would show this is a general solution?

I tried adding just a file, and enabling translation, also enabling description, but i dont see description as a sub field.

----

here are the shots on image field:

1.
default.png

2.
file_translatable.png

3. looking good on the field settings for the content type too.
on_content_type_edit.png

looks ok to me. I'd rtbc it, but I changed the comments in the last patch.

alexh58’s picture

Yea I couldn't find another field that has parts to it like that, and I don't think I can RTBC either ha

saltednut’s picture

Status: Needs review » Needs work
FileSize
10.92 KB
10.79 KB
10.65 KB

Its close but when I first land on a page with everything checked - I still have the ability to uncheck the dependents as they are not disabled unless I interact with the parent checkbox.

Page load:
1920888-all-checked.png

Able to uncheck dependents:
1920888-shouldnt-be-able-to-do.png

If I uncheck then check the parent, then the JS fires...
1920888-after-uncheck-recheck-parent.png

...but all I'd need to do is reload the page to get access back to the disabled fields.
1920888-all-checked.png

alexh58’s picture

Oh good catch. I've made a fix for that as well. Attached is the patch.

alexh58’s picture

Status: Needs work » Needs review
FrancescoQ’s picture

I've tested last patch, it works well!

saltednut’s picture

Status: Needs review » Reviewed & tested by the community

Yes - this is working as expected now. Great!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oh, wicked! I totally caught this when I did a demo of D8MI the other day.

Committed and pushed to 8.x. Thanks!

tstoeckler’s picture

Title: Add some JS to make the dependent groups always checked (and readonly) when the master group is checked translatable » Follow-up: Add some JS to make the dependent groups always checked (and readonly) when the master group is checked translatable
Status: Fixed » Needs work
+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -59,7 +67,9 @@ function _translation_entity_form_language_content_settings_form_alter(array &$f
   $form['#attached']['library'][] = array('translation_entity', 'drupal.translation_entity.admin');
+  $form['#attached']['js'][] = array('data' => drupal_get_path('module', 'translation_entity') . '/translation_entity.admin.js', 'type' => 'file');

This should not have been added to core. Instead of randomly adding a JS file this should either be added to the existing translation entity admin library or a new library should be added. Without this we don't get proper JS dependencies.

YesCT’s picture

Issue tags: +Novice, +needs initial patch

tagging with novice to make an initial patch for the follow-up of moving around the js to another file.

Kevin Morse’s picture

If someone wants to explain what is meant by

make an initial patch for the follow-up of moving around the js to another file

I might have a go at it...

Kevin Morse’s picture

Issue summary: View changes

Updated issue summary, corrected number for which part of original comment in other issue.

ltrain’s picture

Creating a patch with pmelab at DrupalCon

pmelab’s picture

translation_entity.admin.js is already included in the drupal.translation_entity.admin, so we basically removed the javascript file from #attached and added the library instead.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yes, please! Thanks.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Status: Fixed » Closed (fixed)

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

nod_’s picture

Issue tags: +JavaScript

Please tag your issues with "JavaScript" if a patch touches a JS file in any way. This patch reintroduced a bad use of $.each() that was fixed a year ago #2057371: Re-Replace all $.each() with filtered for loop.

Thank you.

nod_’s picture

Issue summary: View changes

Submitting a patch at DrupalCon with pmelab

klonos’s picture

Title: Follow-up: Add some JS to make the dependent groups always checked (and readonly) when the master group is checked translatable » Add some JS to make the dependent groups always checked (and readonly) when the master group is checked translatable
Issue summary: View changes
Parent issue: » #1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget