Problem/Motivation

language-negotiation-configure-form.html.twig hardcodes a number of classes on the <div> that wraps each language_type. This is not very flexible if there is a need to manipulate the attributes for contrib or custom implementations.

Proposed resolution

Instantiate Attribute objects in template_preprocess_language_negotiation_configure_form() so that other modules/themes can add in attributes, and change the hardcoded classes in the template to use addClass().

The code can be manually tested by enabling the Language module and navigating to /admin/config/regional/language/detection.

Remaining tasks

Update patch to add the Attributes as part of the array so that they can be manipulated individually and printed from the Twig template with:

{{ language_type.attributes }}

User interface changes

n/a

API changes

n/a

Original report by @davidhernandez

Move classes out of the preprocess functions and into the Twig templates. Use the addClass() attribute method to add classes in the template. Use the clean_class filter to filter class names, if necessary. Maintain all existing functionality and ensure all existing class names are still in the markup, even ones that are inherited.

See the following issues for more detailed examples:
#2217731: Move field classes out of preprocess and into templates
#2254153: Move node classes out of preprocess and into templates

See this change record for information about using the addClass() method:
https://www.drupal.org/node/2315471

See this change record for more information about the phase 1 process of moving class from preprocess to templates:
https://www.drupal.org/node/2325067

Preprocess Functions Modified

template_preprocess_language_negotiation_configure_form
template_preprocess_language_content_settings_table (There seems to be no template file to go with this?)

Twig Templates Modified

language-negotiation-configure-form.html.twig

Files: 
CommentFileSizeAuthor
#22 interdiff.txt472 bytesCottser
#22 2329847-22.patch2.02 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,269 pass(es). View
#20 drupal-move-language-classes-from-preprocess-to-templates-2329847-20.patch2.06 KBtuutti
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,186 pass(es). View
#20 interdiff-2329847-18-20.txt612 bytestuutti
#18 drupal-move-language-classes-from-preprocess-to-templates-2329847-18.patch2.06 KBtuutti
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,183 pass(es). View
#18 interdiff-2329847-13-18.txt1.99 KBtuutti
#13 interdiff-2329847-11-13.txt688 bytestuutti
#13 drupal-move-language-classes-from-preprocess-to-templates-2329847-13.patch1.92 KBtuutti
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,088 pass(es). View
#11 drupal-move-language-classes-from-preprocess-to-templates-2329847-11.patch1.57 KBtuutti
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,834 pass(es). View
#11 interdiff-2329847-9-11.txt653 bytestuutti
#9 drupal-move-language-classes-from-preprocess-to-templates-2329847-9.patch1.58 KBrachel_norfolk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,612 pass(es). View
#6 drupal-move-language-classes-from-preprocess-to-templates-2329847-4.patch1.58 KBrachel_norfolk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,609 pass(es). View

Comments

davidhernandez’s picture

Issue tags: +FUDK
rachel_norfolk’s picture

Assigned: Unassigned » rachel_norfolk
Status: Active » Needs work
davidhernandez’s picture

It looks like settings table is rendered in code. Is this something that needs to be converted into a Twig template?

rachel_norfolk’s picture

Probably does, yes. Going to upload patch before starting that side of things, I think

Cottser’s picture

It looks like that is a newer theme function (added in #1810386: Create workflow to setup multilingual for entity types, bundles and fields) so it didn't end up under the umbrella of #1757550: [Meta] Convert core theme functions to Twig templates yet. Maybe we could tackle the Twigification + bananafication of that in a new issue under #1757550: [Meta] Convert core theme functions to Twig templates to keep this one streamlined?

rachel_norfolk’s picture

FileSize
1.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,609 pass(es). View

Okay - this is a really basic change and more needs to be done to satisfy #5, most likely in a different issue...

rachel_norfolk’s picture

Assigned: rachel_norfolk » Unassigned
Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Needs work

Thanks @rachel_norfolk! Yes, I agree a separate issue makes the most sense.

This looks like a great start, a few points though:

  1. +++ b/core/modules/language/language.admin.inc
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Template\Attribute;
    
    @@ -88,7 +89,7 @@ function template_preprocess_language_negotiation_configure_form(&$variables) {
    -
    +  $variables['attributes'] = new Attribute();
    

    In brief I think if we call it wrapper_classes in the template then this should probably be wrapper_attributes.

    Longer version: It's not usually necessarily to initialize the attribute object like this. Currently attributes, title_attributes, and content_attributes automatically get initialized as Attribute objects. But in this case it's only so it can be used in the Twig template. A bit of an odd case.

  2. +++ b/core/modules/language/templates/language-negotiation-configure-form.html.twig
    @@ -19,8 +19,16 @@
     #}
    +
    

    The extra blank line here is probably not needed.

  3. +++ b/core/modules/language/templates/language-negotiation-configure-form.html.twig
    @@ -19,8 +19,16 @@
    -  <div class="form-item table-language-group table-{{ language_type.type }}-wrapper">
    +  {%
    +  set wrapper_classes = [
    +  'form-item',
    +  'table-language-group',
    +  'table-' ~ language_type.type|clean_class ~ '-wrapper',
    +  ]
    +  %}
    +  <div {{ attributes.addClass(wrapper_classes) }}>
    

    The alignment and indenting on the set tag is not consistent with what is shown in #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates..

    Don't add a space between <div and printing {{ attributes }}, otherwise you will end up with extra whitespace in your tags and if there are no attributes you'll end up with <div >. The Attribute object automatically prints its own whitespace.

rachel_norfolk’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,612 pass(es). View

Thanks for those @Cottser - I'm only on my 3rd d8 patch and LOTS to learn!! I think an entertaining final night of #fudk helps with the enthusiasm but maybe not so much with my code accuracy!!

I have incorporated the things you have mentioned in #8.

Cottser’s picture

Status: Needs review » Needs work

Great to have you here learning with us! The changes look great, I think there's only one more thing that I would change, otherwise looks great to me.

+++ b/core/modules/language/templates/language-negotiation-configure-form.html.twig
@@ -20,7 +20,14 @@
-  <div class="form-item table-language-group table-{{ language_type.type }}-wrapper">
+  {%
+    set wrapper_classes = [
+      'form-item',
+      'table-language-group',
+      'table-' ~ language_type.type|clean_class ~ '-wrapper',
+    ]
+  %}

The "before" code doesn't put language_type.type through any kind of clean_class type thing, so I think we can remove the clean_class filter from language_type.type for the time being.

tuutti’s picture

Status: Needs work » Needs review
FileSize
653 bytes
1.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,834 pass(es). View
Cottser’s picture

Status: Needs review » Needs work

The interdiff looks backwards but the patch looks correct, thanks @tuutti! Although looking outside the context of the lines changed, I suppose we should also document in the template docblock that the wrapper_attributes variable was added.

You can copy + paste the wrapper_attributes docs from textarea.html.twig.

tuutti’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,088 pass(es). View
688 bytes
Cottser’s picture

Status: Needs review » Reviewed & tested by the community

I manually tested this one on /admin/config/regional/language/detection, looks good and code and docs look good.

Thanks @rachel_norfolk and @tuutti!

Cottser’s picture

Status: Reviewed & tested by the community » Needs review

Talked to @alexpott about this one in IRC, the classes are already in the template – the only thing is they are not using Attribute. I think this type of case will need more discussion as to how we handle them in phase 1 and phase 2.

If we stick with this type of approach of adding Attribute where it wasn't there before, then the wrapper_attributes should probably be language_type.attributes so that it's more flexible/beneficial for later preprocess functions to work with. As it is later preprocess functions can manipulate wrapper_attributes but that would apply to all language_types.

Cottser’s picture

Shifting this to be not a "banana" issue after discussion on tonight's Twig call. This is just a regular ol' cleanup issue now, and I think we just need to implement what I mentioned in the second paragraph of #15 and we could be done.

Cottser’s picture

Issue tags: +Novice
tuutti’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
2.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,183 pass(es). View
dawehner’s picture

+++ b/core/modules/language/language.admin.inc
@@ -84,11 +85,11 @@ function template_preprocess_language_negotiation_configure_form(&$variables) {
+      'attributes' => new Attribute,

Let's use () as well.

tuutti’s picture

FileSize
612 bytes
2.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,186 pass(es). View
Cottser’s picture

Title: Move language classes from preprocess to templates » Use Attribute to replace hard coded classes in language-negotiation-configure-form.html.twig
Cottser’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
2.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,269 pass(es). View
472 bytes

I just added manual testing steps to issue summary. This checks out and the code looks good. Thanks again!

Minor: Attached patch gets rid of the unnecessary line deletion.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed fb4e8cd and pushed to 8.0.x. Thanks!

  • alexpott committed fb4e8cd on 8.0.x
    Issue #2329847 by tuutti, rachel_norfolk, Cottser | davidhernandez: Use...

Status: Fixed » Closed (fixed)

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