Use Attribute to replace hard coded classes in language-negotiation-configure-form.html.twig

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

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?

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

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.

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.

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.

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.

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

Thanks @rachel_norfolk and @tuutti!

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.

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.

+++ 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.

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.

Committed fb4e8cd and pushed to 8.0.x. Thanks!