Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Consider the progress element with fallbacks in javascript.
Comment | File | Size | Author |
---|---|---|---|
#55 | 1229442.patch | 2.04 KB | bleen |
#53 | poll-article-to-div.patch | 1.5 KB | aspilicious |
#38 | 1229442-html5_poll-38.patch | 13.21 KB | amateescu |
#36 | 1229442-html5_poll-36.patch | 13.21 KB | amateescu |
#33 | html5-poll.patch | 11.36 KB | bleen |
Comments
Comment #1
bleen CreditAttribution: bleen commentedtime to start getting my hands dirty ...
Comment #2
bleen CreditAttribution: bleen commentedRe 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?
Comment #3
andrewmacpherson CreditAttribution: andrewmacpherson commentedDefinitely 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):
Comment #4
andrewmacpherson CreditAttribution: andrewmacpherson commentedYes,
<meter>
would seem to be appropriate. The current html5 draft explicitly mentions voting results as an example use-case:Comment #5
bleen CreditAttribution: bleen commentedThis 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.
Comment #6
bleen CreditAttribution: bleen commentedOOohhh ... shiney:
But in FF:
How are we handling this kind of differential?
Comment #7
bleen CreditAttribution: bleen commentedspoke 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
Comment #8
bleen CreditAttribution: bleen commentedthe 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)
Comment #9
bleen CreditAttribution: bleen commentedI accidentally reversed which "bar" tpl shows the total votes... this fixes that
Comment #10
bleen CreditAttribution: bleen commentednote 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
Comment #11
webchickMarking needs review, since there's a patch here.
Comment #12
bleen CreditAttribution: bleen commentednah ... these are not ready for review
/me ...preparing to be slapped by testbot
:)
Comment #13
chrispomeroy CreditAttribution: chrispomeroy commentedreroll of #7, the consolidated patch with the --block tpl.php files removed.
fixed a php syntax typo
Comment #15
JohnAlbinThat'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:
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.
Comment #16
chrispomeroy CreditAttribution: chrispomeroy commentedShould this come out? This should stay in right?
Comment #17
bleen CreditAttribution: bleen commentedThis 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
Comment #18
bleen CreditAttribution: bleen commentedOK .. this is a re-roll of #13
Comment #19
JacineYay! 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.
Comment #21
bleen CreditAttribution: bleen commenteduploaded wrong patch yesterday .. oops
Comment #22
bleen CreditAttribution: bleen commented...and this patch does a bit more simplification of the HTML ...
Comment #23
jessebeach CreditAttribution: jessebeach commentedAdmittedly, 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.
Comment #24
jessebeach CreditAttribution: jessebeach commentedI'd like to see the template files move towards the block.tpl.php model. For example, in block.tpl.php we have
poll-bar.tpl.php looks like this now:
Using the block.tpl.php as the model, I'd love to see something like this:
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"
.Comment #25
JacineAgree 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.
Comment #26
jessebeach CreditAttribution: jessebeach commentedI 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.Comment #27
JacineBut if we use the block template. you've get pluggable attributes for both the wrapper and the title. Seems like enough to me.
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.
Comment #28
jessebeach CreditAttribution: jessebeach commentedSorry 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.
Comment #29
bleen CreditAttribution: bleen commentedI think that we should either keep the bar tpl (although I'm not sure why we would be removing the if($block)):
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?
Comment #30
bleen CreditAttribution: bleen commentedOk .. 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?
Comment #31
bleen CreditAttribution: bleen commentedminor tweak from #30 to make some text properly translatable...
Comment #33
bleen CreditAttribution: bleen commented...and this fixes the tests
Comment #34
bleen CreditAttribution: bleen commentedHere testbot ... here boy
Comment #35
amateescu CreditAttribution: amateescu commentedI 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.
Comment #36
amateescu CreditAttribution: amateescu commentedNew 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
Comment #37
JacineThis 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.
Needs a space after the if.
Comment #38
amateescu CreditAttribution: amateescu commentedFixed.
Comment #39
aspilicious CreditAttribution: aspilicious commentedreviewed and approved by TEAM HTML5
Comment #40
bleen CreditAttribution: bleen commentedwooot
Comment #41
catchWe 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).
Comment #42
xjmTagging.
Comment #43
nicxvan CreditAttribution: nicxvan commentedComment #44
nicxvan CreditAttribution: nicxvan commentedI began the change notice, would someone be willing to write a sample of how to implement a meter?
http://drupal.org/node/1432244
Comment #45
bleen CreditAttribution: bleen commentednickvan ... what exactly are you looking for here?
The original theme meter function was:
Do you need more than this?
Comment #46
xjm@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!
Comment #47
bleen CreditAttribution: bleen commentednickvan: I added a sample theme function to the change alert. Everything else seems to be explained correctly to me...
Comment #48
aspilicious CreditAttribution: aspilicious commentedOk looking good :)
Comment #49
amateescu CreditAttribution: amateescu commentedRestoring title and metadata.
Comment #51
JacineHey, 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?Comment #52
xjmOh, whoops. :) I agree that going back to a div might be better in that case.
Comment #53
aspilicious CreditAttribution: aspilicious commentedAgreed.
Comment #55
bleen CreditAttribution: bleen commentedtry this...
Comment #56
xjmLooks good, thanks @bleen18 and @aspilicious!
Comment #57
catchAlright, back to divs then. Committed/pushed to 8.x, thanks!