Convert theme_meter from theme.inc [#1806558]

First of all, forgive me, this is my first attempt at twig. This patch: #1806426: Incorrect syntax in poll-results.twig causing it to fail needs to be committed before the following can be tested... create a poll and view the results to see the output.

I'm just wondering what needs to be done here to get the attributes ( e.g. data-max="1" data-value="1" ) to actually print in the template. Currently, everything works except for the HTML5 data attributes aren't printed, and the following error displays:

User error: "class" is an invalid render array key in element_children() (line 6251 of core/includes/common.inc).
User error: "class" is an invalid render array key in element_children() (line 6251 of core/includes/common.inc).
User error: "class" is an invalid render array key in element_children() (line 6251 of core/includes/common.inc).

Should a preprocess function be created for theme_meter to handle this portion of theme_meter?

  $attributes = $variables['attributes'];

  foreach (array('form', 'high', 'low', 'max', 'min', 'optimum', 'value') as $attribute) {
    if (!empty($variables[$attribute])) {
      // This function was initially designed for the <meter> tag, but due to
      // the lack of browser and styling support for it, we're currently using
      // it's attributes as HTML5 data attributes.
      $attributes['data-' . $attribute] = $variables[$attribute];
    }
  }

I tried pulling it into the template itself, using something like the following, but I can't get anything but a list of the values to print out.. and it seems to me it shouldn't really be in the template anyway. Advice please?

{% for attribute in ['form', 'high', 'low', 'max', 'min', 'optimum', 'value'] %}
...
{% endfor %}

Comment	File	Size	Author
#16	twig-theme-meter-orig.png	14.35 KB	KrisBulman
#16	twig-theme-meter-mod.png	13.86 KB	KrisBulman
#9	d8tts-theme-meter-twig-1806558-8.patch	2.16 KB	KrisBulman
#7	d8tts-theme-meter-twig-1806558-5.patch	732 bytes	KrisBulman
#5	d8tts-theme-meter-twig-1806558-5.patch	732 bytes	KrisBulman
	d8tts-theme-meter-twig-1750250-1.patch	1.43 KB	KrisBulman

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #0.0

KrisBulman CreditAttribution: KrisBulman commented 8 October 2012 at 15:23

Issue summary:

View changes

added link to patch

Comment #1

Fabianx CreditAttribution: Fabianx commented 8 October 2012 at 15:38

Status:

Needs review

» Needs work

KrisBulman: Thanks a lot for the work!

Please do:

preprocess

$variables['attributes'] = new Attribute($variables['attributes']);

theme:

<tag {{ attributes }}>

or if the class is important:

<tag class="{{ attributes.class }}" {{- attributes }}>

Comment #2

anthonyR CreditAttribution: anthonyR commented 8 October 2012 at 15:57

@Fabianx:
I've seen this pattern before in the sandbox:
<tag class="{{ attributes.class }}" {{- attributes }}>

But this would give an empty class attribute if attributes.class is empty. Shouldn't we add checks for the property around the attribute everywhere this occurs?

Comment #3

Fabianx CreditAttribution: Fabianx commented 8 October 2012 at 16:23

anthoyR: I know.

I think about using {{ attributes }}|merge({ class: ["my-class"] })

instead but I think its too complicated.

Maybe we can keep the "class" and use some twig cleanup magic.

Comment #4

anthonyR CreditAttribution: anthonyR commented 8 October 2012 at 19:37

Sorry for hijacking this issue, but it's something that occurs in almost every template.
This is one of the things we should decide a pattern for so that everyone uses it.
Otherwise we need to patch all the templates again when we decide what to do with it. Separate issue?

Comment #5

KrisBulman CreditAttribution: KrisBulman commented 9 October 2012 at 00:10

Status:

Needs work

» Needs review

File	Size
d8tts-theme-meter-twig-1806558-5.patch	732 bytes

Requested changes are now in and HTML5 attributes appear OK.

Note: there is a problem when adding new questions to a poll, this seems to be coming from theme_table, and should have nothing to do with this patch. Possibly related to #1778968: Convert theme_table to Twig

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /system/ajax
StatusText: OK
ResponseText: Recoverable fatal error: Argument 1 passed to drupal_attributes() must be an array, object given, called in /core/includes/theme.inc on line 1893

Comment #6

Fabianx CreditAttribution: Fabianx commented 9 October 2012 at 00:14

Status:

Needs review

» Needs work

Can you please add the patch?

Comment #7

KrisBulman CreditAttribution: KrisBulman commented 9 October 2012 at 00:16

Status:

Needs work

» Needs review

File	Size
d8tts-theme-meter-twig-1806558-5.patch	732 bytes

That would help!

Comment #8

KrisBulman CreditAttribution: KrisBulman commented 9 October 2012 at 00:16

Status:

Needs review

» Needs work

sorry, let me reroll that. missing the twig template

Comment #9

KrisBulman CreditAttribution: KrisBulman commented 9 October 2012 at 00:24

Status:

Needs work

» Needs review

File	Size
d8tts-theme-meter-twig-1806558-8.patch	2.16 KB

OK, here it is.

Comment #10

Fabianx CreditAttribution: Fabianx commented 9 October 2012 at 13:23

Status:

Needs review

» Needs work

+++ b/core/themes/stark/templates/theme.inc/meter.twigundefined
@@ -0,0 +1,37 @@
+ * - high: A number specifying the range that is considered to be a high
+ *   value.
+ * - low: A number specifying the range that is considered to be a low value.
+ * - max: A number specifying the maximum value of the range.
+ * - min: A number specifying the minimum value of the range.

Hmm, this docs do not match that this is just in the attributes.

I have not seen original function, yet, but this unfortunately needs still some little work.

Good job so far!

Comment #11

KrisBulman CreditAttribution: KrisBulman commented 9 October 2012 at 15:36

this docs do not match that this is just in the attributes.

I don't follow, can you clarify please?

Here is what's in the original function:

/**
 * Returns HTML for a meter.
 *
 * @param $variables
 *   An associative array containing:
 *   - display_value: The textual representation of the meter bar.
 *   - form: A string specifying one or more forms to which the <meter> element
 *     belongs separated by spaces.
 *   - high: A number specifying the range that is considered to be a high
 *     value.
 *   - low: A number specifying the range that is considered to be a low value.
 *   - max: A number specifying the maximum value of the range.
 *   - min: A number specifying the minimum value of the range.
 *   - optimum: A number specifying what value is the optimal value for the
 *     gauge.
 *   - value: A number specifying the current value of the gauge.
 *   - percentage: A number specifying the current percentage of the gauge.
 *   - attributes: Associative array of attributes to be placed in the meter
 *     tag.
 */

API docs: http://api.drupal.org/api/drupal/core!includes!theme.inc/function/theme_meter/8

Comment #12

Fabianx CreditAttribution: Fabianx commented 9 October 2012 at 17:15

Status:

Needs work

» Needs review

Woops, my fault. Really sorry, looks seriously really good.

Could you attach some test code that calls this function for easy testing?

That would be totally great!

Just from looking at it, I'd say its RTBC, but I have not tested, yet.

Comment #13

KrisBulman CreditAttribution: KrisBulman commented 9 October 2012 at 19:43

I'd love to, but not sure how to go about it, do we have any existing tests set up for converted functions to template_preprocess functions?

Here is the original documentation on how to override theme_meter, which dropped the data- from the html5 attribute and moved from a <div> to a <meter> tag.