Move classes out of the preprocess functions and into the Twig templates.

Twig Templates Modified

core/modules/system/templates/time.html.twig
core/modules/datetime/templates/datetime-form.html.twig
core/modules/datetime/templates/datetime-wrapper.html.twig

Preprocess Functions Modified

core/includes/theme.inc: template_preprocess_time
core/modules/datetime/datetime.module: template_preprocess_datetime_form
core/modules/datetime/datetime.module: template_preprocess_datetime_wrapper

CommentFileSizeAuthor
#27 move_datetime_classes-2322287-27.patch3.1 KBlauriii
#27 interdiff-2322287-25-27.txt571 byteslauriii
#25 move_datetime_classes-2322287-25.patch2.81 KBtuutti
#25 interdiff-2322287-23-25.txt383 bytestuutti
#23 interdiff-2322287-17-23.txt855 bytestuutti
#23 move_datetime_classes-2322287-23.patch2.77 KBtuutti
#20 move_datetime_classes-2322287-20.patch2.46 KBtuutti
#20 interdiff-2322287-17-20.txt917 bytestuutti
#17 interdiff-2322287-15-17.txt554 byteslauriii
#17 move_datetime_classes-2322287-17.patch2.86 KBlauriii
#15 move_datetime_classes-2322287-15.patch2.91 KBlauriii
#6 consensus_banana_phase-2322287-6.patch2.28 KBwheatpenny
#4 consensus_banana_phase-2322287-4.patch2.25 KBwheatpenny
#1 consensus_banana_phase-2322287-1.patch2.26 KBwheatpenny
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wheatpenny’s picture

wheatpenny CreditAttribution: wheatpenny commented
FileSize
consensus_banana_phase-2322287-1.patch2.26 KB
wheatpenny’s picture

wheatpenny CreditAttribution: wheatpenny commented
Assigned: wheatpenny » Unassigned
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: consensus_banana_phase-2322287-1.patch, failed testing.

wheatpenny’s picture

wheatpenny CreditAttribution: wheatpenny commented
Status: Needs work » Needs review
FileSize
consensus_banana_phase-2322287-4.patch2.25 KB

Status: Needs review » Needs work

The last submitted patch, 4: consensus_banana_phase-2322287-4.patch, failed testing.

wheatpenny’s picture

wheatpenny CreditAttribution: wheatpenny commented
Status: Needs work » Needs review
FileSize
consensus_banana_phase-2322287-6.patch2.28 KB
wheatpenny’s picture

wheatpenny CreditAttribution: wheatpenny commented
wheatpenny’s picture

wheatpenny CreditAttribution: wheatpenny commented

This patch needs to incorporate template_preprocess_datetime and datetime.html.twig. However, #2088365: Wrong name for datetime.html.twig should be time.html.twig is going to change the datetime.html.twig to time.html.twig. Therefore, I'm going to hold off with this issue until #2088365: Wrong name for datetime.html.twig should be time.html.twig is committed.

To do in the existing patch:

1. trailing comma on array of classes
2. rename "classes" variable to "title_classes" in datetime-wrapper.html.twig.

wheatpenny’s picture

wheatpenny CreditAttribution: wheatpenny commented
Status: Needs review » Needs work
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Title: Consensus Banana Phase 1, Move datetime classes from preprocess to template » Move datetime classes from preprocess to template

#2088365: Wrong name for datetime.html.twig should be time.html.twig is in!

I don't think all these sub-issues need to say 'Consensus Banana', I vote we keep it simple :)

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Issue summary: View changes

Updating the summary to include the time template and preprocess.

wheatpenny’s picture

wheatpenny CreditAttribution: wheatpenny commented
Issue tags: +frontendunited

Updating tags list for Front End United sprint.

wheatpenny’s picture

wheatpenny CreditAttribution: wheatpenny commented

To do:

1. trailing comma on array of classes
2. rename "classes" variable to "title_classes" in datetime-wrapper.html.twig.
3. Move classes from template_preprocess_time to time.html.twig

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Issue tags: +FUDK
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs work » Needs review
FileSize
move_datetime_classes-2322287-15.patch2.91 KB

Rerolled the patch and did the changes proposed in #13.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented
Status: Needs review » Needs work 
+++ b/core/modules/system/templates/time.html.twig
@@ -25,4 +25,9 @@
+{%
+  set classes = [
+    'datetime',
+  ]
+%}
+<time{{ attributes.addClass(classes) }}>{{ html ? text|raw : text }}</time>

This can just be:

<time{{ attributes.addClass('datetime') }}>{{ html ? text|raw: text }}</time>
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
FileSize
move_datetime_classes-2322287-17.patch2.86 KB
interdiff-2322287-15-17.txt554 bytes

Yeah!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs work » Needs review

Sending to testbot also!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work

Looking great overall!

  1. +++ b/core/includes/theme.inc
@@ -867,10 +862,6 @@ function template_preprocess_datetime_form(&$variables) {
-  if (!empty($element['#attributes']['class'])) {
-    $variables['attributes']['class'] = (array) $element['#attributes']['class'];
-  }

    I think we might need to keep this part so other modules can pass things in via the render element. 

  2. +++ b/core/includes/theme.inc
@@ -896,13 +887,11 @@ function template_preprocess_datetime_wrapper(&$variables) {
+    $variables['required'] = 1;

    Can we do TRUE instead of 1 please? That'll be nicer when debugging from the template.

tuutti’s picture

tuutti CreditAttribution: tuutti commented
FileSize
interdiff-2322287-17-20.txt917 bytes
move_datetime_classes-2322287-20.patch2.46 KB
tuutti’s picture

tuutti CreditAttribution: tuutti commented
Status: Needs work » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs review » Needs work 
+++ b/core/includes/theme.inc
@@ -862,6 +862,10 @@ function template_preprocess_datetime_form(&$variables) {
+  $variables['attributes']['class'][] = 'container-inline';

Looks good but this one we don't need anymore! :)

tuutti’s picture

tuutti CreditAttribution: tuutti commented
Status: Needs work » Needs review
FileSize
move_datetime_classes-2322287-23.patch2.77 KB
interdiff-2322287-17-23.txt855 bytes
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented 
+++ b/core/includes/theme.inc
@@ -896,13 +890,11 @@ function template_preprocess_datetime_wrapper(&$variables) {
   // For required datetime fields a 'form-required' class is appended to the
   // label attributes.
   if (!empty($element['#required'])) {
-    $title_attributes['class'][] = 'form-required';
+    $variables['required'] = TRUE;
   }

Can we also explicitly set a FALSE for when #required is empty? Otherwise it won't show up in Twig context for debugging. There may be one or two other banana patches that could use this type of change.

Other than that looks great. Thanks @tuutti!

tuutti’s picture

tuutti CreditAttribution: tuutti commented
FileSize
interdiff-2322287-23-25.txt383 bytes
move_datetime_classes-2322287-25.patch2.81 KB
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work

Oops. And since we're adding the new 'required' variable we should document it in the Twig template :) Something like "A flag indicating whether the field is required or not" maybe.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs work » Needs review
FileSize
interdiff-2322287-25-27.txt571 bytes
move_datetime_classes-2322287-27.patch3.1 KB
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Status: Needs review » Reviewed & tested by the community

I manually tested and everything looks good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 145778c and pushed to 8.0.x. Thanks!

  • alexpott committed 145778c on 8.0.x 
    Issue #2322287 by tuutti, lauriii, wheatpenny: Move datetime classes...

Status: Fixed » Closed (fixed)

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