Problem/Motivation
There is a hardcoded content translation status in ContentTranslationController::overview()
$status = '<span class="status">' . $status . '</span>' . (!empty($translation['outdated']) ? ' <span class="marker">' . $this->t('outdated') . '</span>' : '');
Proposed resolution
Add inline template to move this html to rendering level. see https://www.drupal.org/node/2311123
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? | |
---|---|---|---|---|
Create a patch | Yes | Instructions | ||
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#28 | Screen Shot 2014-08-24 at 18.34.28.png | 93.79 KB | vijaycs85 |
#26 | 2250841-26.patch | 1.55 KB | andypost |
#26 | interdiff.txt | 1.32 KB | andypost |
#16 | status2.png | 15.3 KB | andypost |
#16 | status1.png | 3.71 KB | andypost |
Comments
Comment #1
vlkff CreditAttribution: vlkff commentedComment #2
Buratino42 CreditAttribution: Buratino42 commentedAdd theme function. Hope right understand issue )
Comment #3
Buratino42 CreditAttribution: Buratino42 commentedComment #4
Buratino42 CreditAttribution: Buratino42 commentedComment #5
Buratino42 CreditAttribution: Buratino42 commentedComment #7
vlkff CreditAttribution: vlkff commentedBuratino42, thank you for your work, but your patch from #2 not exactly doing what requested in this issue and require a rework. You just wrapped the hardcoded code in a wrapper function, while we need to create and use a theme function.
Please rework your patch in this way:
* implement a theme using hook_theme
* replace hardcoded $status contents by drupal_render call
* please place in your theme function all status message markup, not only the marker.
Follow links may help you:
https://api.drupal.org/api/drupal/core%21modules%21system%21theme.api.ph...
https://api.drupal.org/api/drupal/core%21modules%21system%21system.api.p...
https://api.drupal.org/api/drupal/core%21includes%21common.inc/function/...
Comment #8
vlkff CreditAttribution: vlkff commentedComment #9
piyuesh23 CreditAttribution: piyuesh23 commentedComment #10
dcam CreditAttribution: dcam commentedUpdating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....
Comment #11
thechanceg CreditAttribution: thechanceg commentedHere is an attempt at a switching it over to hook_theme. It works, but I'm not sure if adding the hooks to the .module file is the commonly accepted approach. I'm still getting used to d8.
Comment #12
surandesilva CreditAttribution: surandesilva commentedWe tested against the latest dev version of Drupal 8 and confirmed that the theme hook worked correctly. Tested content translation module and made sure that the outdated string appeared correctly.
Comment #13
alexpottDo we really need to a theme function for this? Why would someone override this?
Comment #14
tstoecklerAlso, let's not introduce new theme functions when our twig team is so hard at work converting them :-)
Comment #15
star-szrI think @joelpittet would be one of the best people to ask about how this type of thing could fit in to the D8 "markup ecosystem" (I've left him a tell on IRC), but yes it would be nice to not introduce new theme functions. Thanks for the ping @alexpott :)
And yes thank you @tstoeckler, we have done a ton of work around #1757550: [Meta] Convert core theme functions to Twig templates so this would just be adding to that list.
In general IMO it's nice to be able to override markup, thinking of admin themes for this case. But it seems weird to introduce a theme function or template for this, it's so specific (we're trying to get rid of as many of these one-offs as possible when it makes sense) and there really isn't much markup. Hoping there is a better way. It may be significant that this is a "marker", I wonder if mark.html.twig could be a fit for at least part of this or if we could make mark__translation_status or something along those lines.
Comment #16
andypostThis is really annoying bug, how it looks is #2251907: Wrong status on Translate tab for entities
how it looks now
markup
Comment #17
andypostComment #18
andypostComment #19
star-szrCode looks great and I manually tested it, works great. Thank you @andypost! I don't think all the issues fixing double escaping need tests added. RTBC from me.
Comment #20
andypostThe test would be added in #2251907: Wrong status on Translate tab for entities
Comment #21
dawehnerI am confused, #2305799: HTML tags (raw) shown in Status column on Translations page was marked as duplicate of #2317557: renderInline not compatible with twig_auto_reload which now contains that as well.
we can also put all t() calls into the twig itself.
Comment #22
star-szrI did mention that the t stuff could be in Twig in IRC, I'm not sure if that's equivalent to $this->t() or if it matters.
Comment #23
andypost@dawehner There's no difference between
trans
and$this->t()
both are parsed fineMy preference is
t()
because it much clear for translators (users of l.d.o) to get context from code not the templatesComment #24
Gábor HojtsyComment #25
dawehner@andypost
Sure, both are fine, though it feels just better to move this kind of stuff into templates as they don't really belong to the logic on the code.
Comment #26
andypost@dawehner - as you wish ;) template is a bit longer... and less readable
Comment #27
dawehnerThank you!
Comment #28
vijaycs85Tested manually. Looks good. +1 to RTBC.
Comment #29
webchickLooks good.
Committed and pushed to 8.x. Thanks!
Comment #32
jibran