Discovered in #3117217: Decouple core theme dependency on functions in stable.theme
Claro's file-link template always displays file_size, but this should not always be displayed
<span{{ attributes }}>{{ link }} <span class="file__size">({{ file_size }})</span></span>
When file-link used by image_widget, file_size should always be displayed, but there are other uses for this template such as by the file_table formatter, which displays file size in a separate column.
The default file-link template in file module already has logic to not add the file size span when the file_size variable is empty. See #3482024: 'file_table' formatter shows file size twice to provide logic for disabling file size when building a file link.
Fix Claro's file-link template to not add the file size span when file_size is empty. This ensures that file links will not display empty parentheses.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | Screenshot-of-claro-preprocess-image-widget.png | 63.11 KB | niranjan_panem |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !13334
Comments
Comment #9
niranjan_panem commentedIn drupal 11 claro theme suffix for file-size is not added in preprocess_image_widget. Below is the screen shot of it.
Comment #10
bnjmnm@niranjan_panem that is correct because, as the issue summary states Claro's file-link template always displays file_size
The template is where the issue lies
The preprocess in the issue summary was explaining how other themes addressed this in the past.
Comment #11
dafederWhat is the argument against wrapping the whole span in a conditional to check if file_size is set? This is how it is handled in the core twig template for file module. There are other contexts when that variable might be empty, and you wouldn't want to show the parentheses if there's nothing between them. Likewise, if you want to hide it you could just unset that variable via preprocess.
Comment #12
avpadernoThere are cases where
file_sizeis already shown in a different place, for example in a different table column. It is not a matter of checking whetherfile_sizeis set, but showing the same information twice.Comment #13
mfbI incorporated a fix for this Claro bug in a MR for #3482024: 'file_table' formatter shows file size twice, which resolves the related issue re: file module needing logic to turn off file size display when it's already displayed elsewhere.
Comment #15
mfbNever mind, moved the fix here in hopes of faster reviews
Comment #16
smustgrave commentedThis one also needs an issue summary updates, IS mentions
stable_preprocess_image_widgetbut we know stable has been removed so is there a different hook causing this?This is the current proposed solution but the MR appears to be wrapping the template. Hard to tell but comments #10-12 seem to be discussing if this is the approach that should be taken
Comment #17
mfbI removed the obsolete verbiage.
Comment #18
mfbTemplate variables don't have a
$Comment #19
avpadernoThe issue summary says:
It does not seem a matter of checking the value of
file_sizein the template file, but rather to know which formatter is using that template file, or use two different template files.Comment #20
avpaderno(I am interested in this as maintainer of the Seven theme, now removed from Drupal core, and maintained as contributed project.)
Comment #21
mfbSee the following paragraph of the issue summary - this should be handled by #3482024: 'file_table' formatter shows file size twice (a new size toggle available to formatters or other callers).
Comment #22
smustgrave commentedAs a template change will need a CR as contrib themes (like seven) will need to be informed.
Comment #23
mfbAdded a draft CR
Comment #25
smustgrave commentedBelieve this one is good to go. Also think change is small enough to not need test coverage
Comment #26
longwaveBackported down to 10.6.x as an eligible bug fix. I'm also not convinced this needed a change record as it's only a bug fix and other themes can make their own decisions independent of this, but we have one so I'll publish it.
Committed and pushed f33a27a30e4 to main and 0cb6f05cbda to 11.x and f50606671d8 to 11.3.x and 5ea9949c2cd to 10.6.x. Thanks!
Comment #33
longwaveActually no, I'm hiding the change record, it doesn't add anything useful.