Beta phase evaluation
Issue category | Task because clean ups code by using new APIs |
---|---|
Issue priority | Normal because replaces code that works with code that is the current best practice |
Disruption | No disruption |
Problem/Motivation
In TranslateEditForm::buildForm() and PluralString::getTranslationElement(), the use of '#markup' and string concatenation could be replaced with inline_templates.
Proposed resolution
Replace string concatenation with inline_templates where necessary in TranslateEditForm::buildForm() and PluralString::getTranslationElement()
Remaining tasks
Implement. Review.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#30 | html.diff | 5.54 KB | maxocub |
| |||
#27 | interdiff.txt | 2.83 KB | maxocub |
#27 | use_inline_templates-2499651-27.patch | 9.57 KB | maxocub |
#21 | interdiff.txt | 2.5 KB | maxocub |
#21 | use_inline_templates-2499651-21.patch | 8.89 KB | maxocub |
Comments
Comment #1
maxocub CreditAttribution: maxocub commentedPostponed on #2454829: Configuration translation UI does not support plural sources/targets
Comment #2
Gábor HojtsyComment #3
penyaskitoComment #4
maxocub CreditAttribution: maxocub commentedComment #5
andypostComment #6
Gábor HojtsyLooks like this is not being worked on, so removing from sprint. Please add it back if so.
Comment #7
maxocub CreditAttribution: maxocub commentedComment #8
maxocub CreditAttribution: maxocub commentedComment #9
maxocub CreditAttribution: maxocub commentedHere's a first patch. I never used #inline_template before and it's not documented on the Form API page (should I open an issue about that?), so I looked at other usage in core.
Comment #10
andypostI think in most places you can leave that as render array (no need to render)
makes sense to move template definition out of loop
Comment #11
maxocub CreditAttribution: maxocub commentedComment #12
maxocub CreditAttribution: maxocub commented@andypost: Do you mean something like this:
I tried that and it just prints 'array' on the page. I guess I could just use the 'render' function instead of calling the 'renderInline' from twig, would that makes it better?
Or maybe I am not using the render array correctly?
Comment #13
andypost@maxocub please try to change render array to prevent rendering, it looks really strange that we need render in build
Comment #14
maxocub CreditAttribution: maxocub commentedNew patch where I replace the 'item' form elements by 'inline_template'. We do lose some auto generated CSS classes and IDs, but AFAIK it doesn't matter, since it's just markup to show the source string to be translated.
Comment #17
maxocub CreditAttribution: maxocub commentedThis test was failing on #2571655: ConfigNamesMapper::hasTranslatable has flawed logic too, and I corrected it there too, not sure what's the best approach for this situation.
Comment #19
maxocub CreditAttribution: maxocub commentedOh, silly me, the test is looking for CSS IDs that no longer exists.
Comment #21
maxocub CreditAttribution: maxocub commentedI really don't know if it's OK to set an ID this way, but I added it on the inline template to be used with the test.
Comment #22
penyaskitoIt's totally fine, nice cleanup :)
Comment #23
penyaskitoAdded beta evaluation template.
Comment #24
penyaskitoTested that works correctly.
Comment #25
penyaskitoWhy is this needed? If we are changing the HTML element ids this way, we may have duplicated HTML ids which makes our HTML invalid.
Comment #26
maxocub CreditAttribution: maxocub commentedRight, I forgot there may be more than one plural variant field on this long form, so there would be duplicated IDs. I'll find another way to test it.
Comment #27
maxocub CreditAttribution: maxocub commentedI removed the potentially duplicated IDs and modified the test to count the number of source elements with xpath.
Comment #28
maxocub CreditAttribution: maxocub commentedComment #29
Gábor HojtsyThe modified code seems to be doing a different thing now? The fieldset title is the definition label and then it has a child element instead of the title contain the text?
Same here, the template seems to be in a different structure?!
Comment #30
maxocub CreditAttribution: maxocub commentedSome before/after screenshots showing the visual haven't changed.
Also, a diff of the HTML structure changes.
PlurialVariants.php
What changed here is that
<span class="visually-hidden>(English)</span>
moved from the fieldset-lengend span to the fieldset-wrapper div. Also, some CSS ids ans classes are gone, but I don't see any side effects.TranslateEditForm.php
What changed here is that the
<span lang="en">
is not anymore around the whole div, but around each source strings, which is more like normal singular strings are presented in this form. There's also some CSS ids ans classes that are gone.Comment #32
maxocub CreditAttribution: maxocub commentedComment #33
Gábor HojtsyAll righto, I misread something then, let's get this in :) Thanks a lot!
Comment #34
alexpottAren't these missing the
for
attribute - I think that for was wrong before though - so not sure that this matters.Comment #35
maxocub CreditAttribution: maxocub commentedThe label tags without a for attribute are on the source side. So they are not associated with an input field, but with a span. The label tags on the translation side have a for attribute with the id of their input field.
Before this patch, the two label tags (the source one and the translation one) had both the same for attribute pointing to the translation's input field.
Maybe we should use a tag other than label for those without an input field?
Comment #37
maxocub CreditAttribution: maxocub commentedIs there still work to be done here or can it be RTBC?
Comment #38
maxocub CreditAttribution: maxocub commentedComment #50
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Taking a look at the code for 10.1 and I see now it's using FormattableMarkup() does that resolve the issue here?
If so think this can be closed and credit moved over to #2958429: Properly deprecate SafeMarkup::format()