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
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.
Comments
Comment #1
alexh58 CreditAttribution: alexh58 commentedHi 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.
Comment #3
YesCT CreditAttribution: YesCT commentedfunctional review (done easily with the dreditor simplytest.me button):
on the content type manage field edit tab for an image:
it's nice!
please make it also do that on the overall content language settings page (under Configuration)
Comment #4
YesCT CreditAttribution: YesCT commentedThanks! I would not have known where to start to implement that js!
a standards review.
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.
inline should also be sentence that ends in a period.
use spaces to indent instead of tabs.
http://drupal.org/coding-standards#indenting
use just one newline to separate things.
Comment #5
saltednutShould this be more generic if there are other places (besides File fields) where dependents need to be checked?
Comment #6
alexh58 CreditAttribution: alexh58 commentedAwesome. 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!
Comment #7
YesCT CreditAttribution: YesCT commentedjust re-wrapped the comments to be 80 chars.
manual testing next.
Comment #8
YesCT CreditAttribution: YesCT commentedis 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.
2.
3. looking good on the field settings for the content type too.
looks ok to me. I'd rtbc it, but I changed the comments in the last patch.
Comment #9
alexh58 CreditAttribution: alexh58 commentedYea I couldn't find another field that has parts to it like that, and I don't think I can RTBC either ha
Comment #10
saltednutIts 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:
Able to uncheck dependents:
If I uncheck then check the parent, then the JS fires...
...but all I'd need to do is reload the page to get access back to the disabled fields.
Comment #11
alexh58 CreditAttribution: alexh58 commentedOh good catch. I've made a fix for that as well. Attached is the patch.
Comment #12
alexh58 CreditAttribution: alexh58 commentedComment #13
FrancescoQ CreditAttribution: FrancescoQ commentedI've tested last patch, it works well!
Comment #14
saltednutYes - this is working as expected now. Great!
Comment #15
webchickOh, wicked! I totally caught this when I did a demo of D8MI the other day.
Committed and pushed to 8.x. Thanks!
Comment #16
tstoecklerThis 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.
Comment #17
YesCT CreditAttribution: YesCT commentedtagging with novice to make an initial patch for the follow-up of moving around the js to another file.
Comment #18
Kevin Morse CreditAttribution: Kevin Morse commentedIf someone wants to explain what is meant by
I might have a go at it...
Comment #18.0
Kevin Morse CreditAttribution: Kevin Morse commentedUpdated issue summary, corrected number for which part of original comment in other issue.
Comment #19
ltrainCreating a patch with pmelab at DrupalCon
Comment #20
pmelab CreditAttribution: pmelab commentedtranslation_entity.admin.js
is already included in thedrupal.translation_entity.admin
, so we basically removed the javascript file from #attached and added the library instead.Comment #21
tstoecklerYes, please! Thanks.
Comment #22
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #24
nod_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.
Comment #24.0
nod_Submitting a patch at DrupalCon with pmelab
Comment #25
klonos