Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bleen’s picture

Assigned: Unassigned » bleen

time to start getting my hands dirty ...

bleen’s picture

Re considering the "progress" tag ... I think the meter tag is actually more appropriate for the poll module: http://www.w3schools.com/html5/tag_meter.asp

Any strong opinions?

andrewmacpherson’s picture

Definitely not <progress>, as that is specifically for indicating the progress of a task. The poll bar represents the results of previous user-actions(s), not a task which is currently in progress.

From the current html5 draft (emphasis mine):

The progress element represents the completion progress of a task.
[...]
The progress element is the wrong element to use for something that is just a gauge, as opposed to task progress."

andrewmacpherson’s picture

Yes, <meter> would seem to be appropriate. The current html5 draft explicitly mentions voting results as an example use-case:

The meter element represents a scalar measurement within a known range, or a fractional value; for example disk usage, the relevance of a query result, or the fraction of a voting population to have selected a particular candidate.

bleen’s picture

FileSize
1.29 KB

This is a simple "getting started" patch ... it only handles the html, not the necessary css changes.

I dont see a big advantage in having two tpls here ... cant these be combined into one with a simple variable that indicates wether to show the number of votes or not? Additionally, I would love to get rid of that inline style ... looking into some possible solutions to that.

bleen’s picture

FileSize
1.64 KB

OOohhh ... shiney:
How many? | d8

But in FF:
How many? | d8

How are we handling this kind of differential?

bleen’s picture

Title: Convert poll-bar.tpl.php to HTML5 » Convert poll tpls and markup to HTML5
FileSize
6.25 KB

spoke to Jacine and I'm going to consolidate the poll issues here ...

This patch combines the stuff Ive been working on as well as chrispomeroy's work in #1229440: Convert poll-bar--block.tpl.php to HTML5 and #1190172: Convert poll-results--block.tpl.php to HTML5

bleen’s picture

the patch in #8 removes the --block.tpl files from poll as they are 95% the same as their non-block counterparts. On the other hand, lets say I want my block version of polls to have a vastly different markup then the normal version (maybe I want the normal version to be two-col, with the question on teh left really big and the choices on the right) … I cant (easily) do that with one tpl. I would have to add a new tpl myself.

Thoughts?

(this patch is the same as #7 but leaves those files in. just so we can move on once a decision is made)

bleen’s picture

I accidentally reversed which "bar" tpl shows the total votes... this fixes that

bleen’s picture

note that this and #1217032: Clean up the CSS for Poll module are going to bump heads ... no big deal, but which ever gets committed first will require a reroll of the other

webchick’s picture

Status: Active » Needs review

Marking needs review, since there's a patch here.

bleen’s picture

nah ... these are not ready for review

/me ...preparing to be slapped by testbot

:)

chrispomeroy’s picture

reroll of #7, the consolidated patch with the --block tpl.php files removed.
fixed a php syntax typo

Status: Needs review » Needs work

The last submitted patch, 1229442-poll-tpls-html5-with-block-tpls-removed-13.patch, failed testing.

JohnAlbin’s picture

the patch in #8 removes the --block.tpl files from poll as they are 95% the same as their non-block counterparts. On the other hand, lets say I want my block version of polls to have a vastly different markup then the normal version (maybe I want the normal version to be two-col, with the question on teh left really big and the choices on the right) … I cant (easily) do that with one tpl. I would have to add a new tpl myself.

That's correct. What's the problem? Just because you've removed the --block.tpl files doesn't mean that a themer can't go and create them themselves.

Oh, wait. I see the patch in #13 does remove the ability of a themer to add a poll-bar--block.tpl.php to their theme.

Please remove this section from the patch:

@@ -903,10 +890,6 @@ function template_preprocess_poll_results(&$variables) {
     $variables['cancel_form'] = drupal_render($elements);
   }
   $variables['title'] = check_plain($variables['raw_title']);
-
-  if ($variables['block']) {
-    $variables['theme_hook_suggestions'][] = 'poll_results__block';
-  }
 }
 
 /**
@@ -919,9 +902,6 @@ function template_preprocess_poll_results(&$variables) {
  * @see theme_poll_bar()
  */
 function template_preprocess_poll_bar(&$variables) {
-  if ($variables['block']) {
-    $variables['theme_hook_suggestions'][] = 'poll_bar__block';
-  }
   $variables['title'] = check_plain($variables['title']);
   $variables['percentage'] = round($variables['votes'] * 100 / max($variables['total_votes'], 1));
 }
 

That bit of code adds "poll_results__block" and "poll_bar__block" to the list of theme hook suggestions and is essential in order to allow themers to have a template file specific to the block content. Don't remove that code.

chrispomeroy’s picture

Should this come out? This should stay in right?

--- a/modules/poll/poll.module
+++ b/modules/poll/poll.module
@@ -47,20 +47,7 @@ function poll_theme() {
       'variables' => array('title' => NULL, 'votes' => NULL, 'total_votes' => NULL, 'vote' => NULL, 'block' => NULL),
     ),
   );
-  // The theme system automatically discovers the theme's functions and
-  // templates that implement more targeted "suggestions" of generic theme
-  // hooks. But suggestions implemented by a module must be explicitly
-  // registered.
-  $theme_hooks += array(
-    'poll_results__block' => array(
-      'template' => 'poll-results--block',
-      'variables' => $theme_hooks['poll_results']['variables'],
-    ),
-    'poll_bar__block' => array(
-      'template' => 'poll-bar--block',
-      'variables' => $theme_hooks['poll_bar']['variables'],
-    ),
-  );
+
   return $theme_hooks;
 }
 
bleen’s picture

This should remain in the patch ... this is where we register those two templates we are nixing with Drupal

Also, if you want to take a look at why testbot is unhappy, take a look at poll.test line 388 ... Ill be looking at this again tonight, so if you have questions just post them here or find me on IRC

bleen’s picture

Status: Needs work » Needs review
FileSize
3.6 KB

OK .. this is a re-roll of #13

  • re-rolled for the directory restructure
  • put back theme suggestions (re #15)
  • a small tweak so that tests should pass
Jacine’s picture

Status: Needs work » Needs review

Yay! This looks great!

It also looks like we might have an actual use case for some sort of polyfill action here. LOL. I'm curious what this looks like with styling applied to it. I'll test and play around with it on Sunday during office hours.

Status: Needs review » Needs work

The last submitted patch, poll-with-block-tpls.patch, failed testing.

bleen’s picture

FileSize
6.48 KB

uploaded wrong patch yesterday .. oops

bleen’s picture

FileSize
7.01 KB

...and this patch does a bit more simplification of the HTML ...

jessebeach’s picture

Admittedly, I may not know enough about theme suggestions to venture an opinion, but here goes: I don't understand why we're adding hooks for block version templates. I agree we should get rid of the templates to promote clean core modules. I also think we should take the next step and get rid of the code that suggests hooks for template files that don't exist.

As far as I can tell, if I create a poll, I don't get a block for that poll. So if I go through the trouble of creating a module that instantiates a block for my poll, I most likely won't need the ease of block hooks already in the poll module. More likely I won't realize these hooks are there (because the templates don't exist) and I'll make the hook suggestions in my module, duplicating code.

jessebeach’s picture

I'd like to see the template files move towards the block.tpl.php model. For example, in block.tpl.php we have

<div id="<?php print $block_html_id; ?>" class="<?php print $classes; ?>"<?php print $attributes; ?>>

poll-bar.tpl.php looks like this now:

<div class="choice-title"><?php print $title; ?></div>
<meter class="bar" min="0" max="<?php print $total_votes; ?>" value="<?php print $votes; ?>">
  <?php print $percentage; ?>%
  <?php if ($block): ?>
    (<?php print format_plural($votes, '1 vote', '@count votes'); ?>)
  <?php endif; ?>
</meter>

Using the block.tpl.php as the model, I'd love to see something like this:

<div class="<?php print $classes; ?>"<?php print $attributes; ?>>
  <div class="<?php print $title_classes; ?>"><?php print $title; ?></div>
  <meter class="<?php print $meter_classes; ?>"<?php $meter_attributes; ?>>
    <?php print $percentage; ?>
  </meter>
</div>

Where the toggled display that hinges on $block is removed and the hard-coded '%' is moved into the template_preprocess function where it can be passed through t(). We also provide module developers a means to add classes and attributes without overriding the templates.

Providing $meter_attributes instead of hard-coding the min and max attributes lets developers vary these values in code, with sensible defaults i.e. min="0".

Jacine’s picture

Status: Needs review » Needs work

Agree with #24 regarding using the block template, and if we do, we get the title attributes for free, which would be good. However, I think using variables for attributes and classes on the <meter> itself is taking it one step too far.

In general though, I don't understand why we are jumping through so many hoops to special case this. What am I missing? The more that I think about it, I wonder why we even need a template here? How likely/frequently will themers to want to change this code? I don't know that I ever have.

jessebeach’s picture

I would be satisfied to see the classes rolled up into variables. Adding a class or two to an element in a template is a foreseeable use case. If adding a class to a template requires copy-and-pasting the entire template, then these templates seem to fail.

@Jacine, I've never seen these templates before today. If they melted into render arrays, would anyone care? Perhaps we really just need a theme_meter function to render the <meter> element.

Jacine’s picture

I would be satisfied to see the classes rolled up into variables. Adding a class or two to an element in a template is a foreseeable use case. If adding a class to a template requires copy-and-pasting the entire template, then these templates seem to fail.

But if we use the block template. you've get pluggable attributes for both the wrapper and the title. Seems like enough to me.

@Jacine, I've never seen these templates before today. If they melted into render arrays, would anyone care? Perhaps we really just need a theme_meter function to render the element.

I dunno... I don't think we need a theme_meter() function. This is probably the only place we'll use it in core, and in general it think it's going to be one of those rarely used elements. These templates are really old, and IMO a disaster. I know I wouldn't miss them, but not sure about everyone else.

jessebeach’s picture

But if we use the block template. you've get pluggable attributes for both the wrapper and the title. Seems like enough to me.

Sorry sorry. I don't think I was clear. I'd like to use the block template as a model. It's an arbitrary choice. The intent is to make the templates uniform in how they handle classes and attributes so that a themer or developer can expect to add values to these attributes in the same way regardless of the core module they're working with.

bleen’s picture

I think that we should either keep the bar tpl (although I'm not sure why we would be removing the if($block)):

<?php
<div class="<?php print $classes; ?>"<?php print $attributes; ?>>
  <div class="choice-title"><?php print $title; ?></div>
  <meter class="<?php print $meter_classes; ?>"<?php $meter_attributes; ?>>
    <?php print $percentage; ?>
    <?php if ($block): ?>
      (<?php print format_plural($votes, '1 vote', '@count votes'); ?>)
    <?php endif; ?>
  </meter>
</div>
?>

OR

just bite the bullet and create a theme_meter and then just nix this tpl in favor of a render array...

OR

we could still use a render array and just use theme_html_tag()

I think I like the render array option best .. thoughts?

bleen’s picture

Status: Needs work » Needs review
FileSize
11.23 KB

Ok .. this is a patch that taks the 2nd option from #29. It includes a brand new function called theme_meter() and it kills the poll-bar templates in favor of a render array.

I would like to go a step further and kill the poll-results templates in favor of a render array as well ... thoughts?

bleen’s picture

FileSize
11.28 KB

minor tweak from #30 to make some text properly translatable...

Status: Needs review » Needs work

The last submitted patch, html5-poll.patch, failed testing.

bleen’s picture

FileSize
11.36 KB

...and this fixes the tests

bleen’s picture

Status: Needs work » Needs review

Here testbot ... here boy

amateescu’s picture

Status: Needs review » Needs work

I have done some research on the <meter> tag, and these are my findings:

http://caniuse.com/#search=meter
http://www.steveworkman.com/html5-2/2009/my-problem-with-html-5-styling-...
http://stackoverflow.com/questions/7228488/style-meter-tag-in-latest-ope...
http://stackoverflow.com/questions/8094835/how-to-style-html5-meter-tag

To sum it up: The <meter> tag is only supported in Chrome and Opera, and it's quite hard to style (not even possible in Opera).

However, I *really* like the template clean-up from this patch, so I propose that the theme_meter() function goes back to our old divs, and keep everything else :)

@30, about killing the poll-results template: two very good follow-ups for this issue are #259725: poll_view_results() should be a theme function and #1315616: A definition list for the poll results, so let's not make perfect the enemy of good and go with what we have in #33 for now.

amateescu’s picture

Status: Needs work » Needs review
FileSize
13.21 KB

New patch based on the thoughts from #35.

Edit: What I really like about the new theme_meter() function is that we can also use it for the password strength indicator :D

Jacine’s picture

Status: Needs review » Needs work

This looks good to me, though I have not actually tested it. Thanks so much for posting all the research you did on this @amateescu!

Found a minor code style issue.

+++ b/core/modules/poll/poll-results.tpl.phpundefined
@@ -25,4 +28,7 @@
+<?php if($block): ?>

Needs a space after the if.

amateescu’s picture

Assigned: bleen » Unassigned
Status: Needs work » Needs review
FileSize
13.21 KB

Fixed.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

reviewed and approved by TEAM HTML5

bleen’s picture

wooot

catch’s picture

Title: Convert poll tpls and markup to HTML5 » Change notification for: Convert poll tpls and markup to HTML5
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

We really need to find a better place to put assorted theme functions that theme.inc, but I can't find the issue for that to link to at the moment.

Patch looks great though, committed/pushed to 8.x.

Let's add a change notification for the new theme function (and probably document the removed stuff from poll there as well).

xjm’s picture

Issue tags: +Needs change record

Tagging.

nicxvan’s picture

Assigned: Unassigned » nicxvan
nicxvan’s picture

Status: Active » Needs review

I began the change notice, would someone be willing to write a sample of how to implement a meter?
http://drupal.org/node/1432244

bleen’s picture

nickvan ... what exactly are you looking for here?

The original theme meter function was:

 /**
+ * Returns HTML for an 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.
+ *   - attributes: Associative array of attributes to be placed in the meter
+ *     tag.
+ */
+function theme_meter($variables) {
+  $attributes = $variables['attributes'];
+
+  foreach (array('form', 'high', 'low', 'max', 'min', 'optimum', 'value') as $attrib) {
+    if (!is_null($variables[$attrib])) {
+      $attributes[$attrib] = $variables[$attrib];
+    }
+  }
+
+  $output = '<meter' . drupal_attributes($attributes) . '>' . $variables['display_value'] . '</meter>';
+
+  return $output;
+}

Do you need more than this?

xjm’s picture

@bleen18: @nicxvan has authored a change notice for this issue:
http://drupal.org/node/1432244
It would be good if someone who worked on the patch here could review that change notice. It would also be helpful to write a little example snippet of how a developer or themer could use the new functionality added by the patch, and add it to the change notice. See, for example, this change notice for a different issue:
http://drupal.org/node/1430892

Thanks!

bleen’s picture

nickvan: I added a sample theme function to the change alert. Everything else seems to be explained correctly to me...

aspilicious’s picture

Status: Needs review » Fixed

Ok looking good :)

amateescu’s picture

Title: Change notification for: Convert poll tpls and markup to HTML5 » Convert poll tpls and markup to HTML5
Priority: Critical » Normal
Issue tags: -Needs change record

Restoring title and metadata.

Status: Fixed » Closed (fixed)

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

Jacine’s picture

Status: Closed (fixed) » Needs review

Hey, I just realized that this patch introduced nested <article> tags for nodes. I realize the only reason that a wrapper exists here is because this content is used both inside a block and the node, but the current markup isn't exactly ideal. I'm wondering if it wouldn't it be better to leave this a <div class="poll"> for this case?

xjm’s picture

Oh, whoops. :) I agree that going back to a div might be better in that case.

aspilicious’s picture

FileSize
1.5 KB

Agreed.

Status: Needs review » Needs work

The last submitted patch, poll-article-to-div.patch, failed testing.

bleen’s picture

Status: Needs work » Needs review
FileSize
2.04 KB

try this...

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks @bleen18 and @aspilicious!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Alright, back to divs then. Committed/pushed to 8.x, thanks!

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