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

Comments

wheatpenny’s picture

FileSize
2.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,762 pass(es), 1 fail(s), and 348 exception(s). View
wheatpenny’s picture

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

Status: Needs work » Needs review
FileSize
2.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,823 pass(es), 0 fail(s), and 346 exception(s). View

Status: Needs review » Needs work

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

wheatpenny’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,356 pass(es). View
wheatpenny’s picture

wheatpenny’s picture

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

Status: Needs review » Needs work
Cottser’s picture

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

Issue summary: View changes

Updating the summary to include the time template and preprocess.

wheatpenny’s picture

Issue tags: +frontendunited

Updating tags list for Front End United sprint.

wheatpenny’s picture

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

Issue tags: +FUDK
lauriii’s picture

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

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

mdrummond’s picture

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

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

Yeah!

lauriii’s picture

Status: Needs work » Needs review

Sending to testbot also!

Cottser’s picture

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

FileSize
917 bytes
2.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,834 pass(es). View
tuutti’s picture

Status: Needs work » Needs review
lauriii’s picture

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

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

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

FileSize
383 bytes
2.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,866 pass(es). View
Cottser’s picture

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

Status: Needs work » Needs review
FileSize
571 bytes
3.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,330 pass(es). View
davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

I manually tested and everything looks good.

alexpott’s picture

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.