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_fieldset
template_preprocess_radios
template_preprocess_checkboxes
template_preprocess_input
template_preprocess_textarea
template_preprocess_form_element
template_preprocess_form_element_label
Twig Templates Modified
fieldset.html.twig
radios.html.twig
checkboxes.html.twig
input.html.twig
textarea.html.twig
form-element.html.twig
form-element-label.html.twig
Screenshots
Before:
After
Comment | File | Size | Author |
---|---|---|---|
#32 | Screen Shot 2014-10-04 at 16.32.39.png | 88.92 KB | lauriii |
#32 | Screen Shot 2014-10-04 at 16.28.57.png | 82.99 KB | lauriii |
#30 | 2329901-30.patch | 16.24 KB | lauriii |
#30 | interdiff-2329901-27-30.txt | 1.25 KB | lauriii |
#5 | interdiff-2329901-3-5.txt | 1.49 KB | lanchez |
Comments
Comment #1
davidhernandezComment #2
lanchez CreditAttribution: lanchez commentedStarted to look into this.
Comment #3
lanchez CreditAttribution: lanchez commentedOk so, I moved all the classes away from form.inc and also one processing from CompositeFormElementTrait.php. There will most probably be problems with inherited classes, but I tried to be careful not to miss anything. Some elements still have hardcoded classes, but they come from element rendering php files, like for example checkboxes form-checkbox class. Also classes might be in different order so some tests might fail because of that. Lets see..
Comment #5
lanchez CreditAttribution: lanchez commentedFixed textarea required attribute and also I had accidentally removed for example aria attributes. Now this should pass tests.
Comment #7
lanchez CreditAttribution: lanchez commentedOk so fixed the (hopefully) last failing test.
There is a general "problem" with adding classes in twig template with addClass if there is no classes in attributes before. The classes goes always to the back of the attributes array and gets printed last. I dont know what standards say about that, but I at least would like them to be after just after possible id attribute.
Comment #8
hass CreditAttribution: hass commentedThat looks very dirty way, but I'm new to twig. The preprocess from past allowed us to add a class without changing the template. How can this done here? I do not like to clone the twig template in my themes if i can add a class in a preprocess function. That looks like a big WTF to me.
Comment #9
star-szr@hass - this doesn't prevent anyone (modules or themes) from adding classes in preprocess. It just moves the default class declarations to the template.
Comment #10
hass CreditAttribution: hass commentedDo we have tests for this? I just ask as D7 has several bugs and do not allow adding additional classes in several preprocess functions. See #1114398: Form element & Form element label theming is broken
Comment #11
star-szrNo, it's not the responsibility of this meta to add tests or fix preprocess bugs. I don't think this meta would ever get done in time if that were the case. We're just moving the class declarations around.
Comment #12
hass CreditAttribution: hass commentedI fear that you may fix the issue by the migration automatically... but testing the case would be good so it does not break again in D9.
Comment #13
lauriiiOverally good job while working on this! We still need to work some more to get clear documentation to Twig templates.
I also think we should use variable instead when we have logic while adding classes
Comment #14
lanchez CreditAttribution: lanchez commentedThanks Lauriii. Here's a patch that moves that one class definition to an twig variable and also modifies some comments.
Too bad that I have deleted by mistake a custom module that generated a nice form to check all this functionality :/
Comment #15
lanchez CreditAttribution: lanchez commentedComment #16
lauriiiI overlooked the patch and there's still few minor documentation issues left:
These shouldn't have apostrophe ( http://dictionary.reference.com/browse/its )
This could include example. Look how its done on label_display
I also created a sandbox module for testing different types of forms. The modules isn't 100% yet so It has to be pushed further later.
Comment #17
bfr CreditAttribution: bfr commentedLauriii, Are you aware of the Examples project? That would most likely be suitable home for your form examples.
Edit: And here's the meta issue: #1880976: [meta] Port examples (including submodules) to D9.4+
Comment #18
lanchez CreditAttribution: lanchez commentedI'll make a new patch on Saturday.
Comment #19
tim.plunkettThis is such a massive DX regression. Can you please leave FAPI alone?
Comment #20
star-szrNeeds a bit more discussion but it was brought up on IRC that fieldset.html.twig probably shouldn't have knowledge of radios and checkboxes coded into it.
Comment #21
lanchez CreditAttribution: lanchez commentedMmkay, so do I continue pathing or should we plan something else?
Comment #22
lauriiiWent a bit back with the changes. I removed the type from fieldset.html.twig and left the functionality now to
CompositeFormElementTrait::preRenderCompositeFormElement
.Comment #25
David Hernández CreditAttribution: David Hernández commentedI've reviewed it a bit and the only thing I could find is a comment that I think it needs to be updated too:
On core/includes/form.inc template_preprocess_form_element:
We can't forget the comment, shouldn't this be moved into the twig template too?
The rest looks fine to me. Anyways, leaving it as needs review, as I'm not really experienced on the frontend / banana stuff.
Comment #27
lauriiiI removed the comment now since I think there's no need for documenting that anymore because themers can easily see the classes in top of template
Comment #28
David Hernández CreditAttribution: David Hernández commentedUpdating it to RTBC.
Well done!
Comment #29
alexpottIt looks like we are losing the required marker here? Did this variable not exist and it now does and is a bool. If so the docs in fieldset.html.twig are now wrong.
Comment #30
lauriiiAdded required class and fixed docs
Comment #31
lauriiiComment #32
lauriiiComment #33
Fabianx CreditAttribution: Fabianx commentedInterdiff looks great to me, screenshots proof the functionality.
I _assume_ that the required boolean works correctly.
Back to RTBC! Thanks!
Comment #34
lauriiiPatch & screenshots proofs that
.form-required
is being added so therequired
boolean is working.Comment #35
alexpott#29
required
here is not a boolean but text that is appearing on the page. To be sure we are not breaking this it would be nice to see a required fieldset both and after this change.Comment #36
Fabianx CreditAttribution: Fabianx commented#35: Isn't that what the screenshots provide?
Comment #37
alexpott#36 but we seem to be printing
required
and now we are not.Comment #38
lauriiiI dont know what for the required variable was there but its empty even if the fieldset has been set required
Comment #39
Fabianx CreditAttribution: Fabianx commentedI think required was before referencing an empty variable, which was a bug.
Twig silently fails on undefined variables per agreement on twig team, so it was just outputting nothing ...
@alexpott: Thanks for your attention to detail!
Comment #40
alexpottCommitted 1256191 and pushed to 8.0.x. Thanks!