Posted by jcmarco on June 23, 2010 at 6:26pm
| Project: | Advanced Poll |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
I was working in the theming and js issues I had with binary polls, write-in's and javascript issues.
Fixes and changes done:
unified the template theming in order to allow overriding templates in custom themes,
clean up the js that was working wrong with the binary polls and write-in's
moved drupal commands to preprocess functions
removed unnecesary jquery_ui libraries
I have tested binary and ranking polls, with and without write-in's and theming and js work fine
| Attachment | Size |
|---|---|
| advpol_themes.patch | 10.2 KB |
Comments
#1
Without testing it, i think that's the right direction.
Additionally here some comments.
+++ advpoll-display-binary-form.tpl.php@@ -0,0 +1,27 @@
+ <?php if (isset($writein_choice)) { ?>
...
+ <?php } ?>
usual notation for theming .tpl.php files for conditions use different markup like
<?php if (isset($writein_choice)): ?>...
<?php endif; ?>
+++ advpoll-display-binary-form.tpl.php@@ -0,0 +1,27 @@
+ <br clear="left" />
+++ advpoll-display-ranking-form.tpl.php
@@ -12,29 +12,24 @@
+ <?php print $form_submit ?>
+ <br clear="left" />
I don't think there's a clear="left" according to w3c stds. is it?
+++ advpoll-vote.js@@ -165,7 +152,7 @@ Drupal.behaviors.rankingDragAndDrop = function(context) {
+ Drupal.attachBehaviors($existingChoicesTable);
this might be already fixed in current dev if i remember correctly.
+++ advpoll.module@@ -1661,6 +1658,13 @@ function advpoll_theme() {
+ 'advpoll_voting_binary_form' => array(
+ 'template' => 'advpoll-display-binary-form',
+ 'file' => 'modes/binary.inc',
+ 'arguments' => array(
+ 'form' => NULL,
+ )
regarding pseudo-pluggability of modes folder (see many parts of the module) it is pretty strange to see this binary-mode specific theming in the main file...
+++ advpoll.module
@@ -1661,6 +1658,13 @@ function advpoll_theme() {
+ ), ¶
...
+++ modes/ranking.inc
@@ -171,28 +171,27 @@ function advpoll_voting_ranking_form(&$form_state, &$node, $teaser, $page, $stat
+ ¶
note/remove trailing spaces
+++ modes/ranking.inc@@ -171,28 +171,27 @@ function advpoll_voting_ranking_form(&$form_state, &$node, $teaser, $page, $stat
+ drupal_add_tabledrag($form['#id'] . '-table', 'order', 'self', 'advpoll-choice-order', NULL, NULL, FALSE);
Generally - if you load js in hook_form or form_alter this only gets executed in build process. In case of any form_validation error those won't be loaded correctly in further loading cycles. If there's never a potential validation fail - this might be OK. In any other case you should add such js/css things in something like a #after_build..
#2
I was trying to have a functional module before starting making changes in code, style or organization.
There are many issues, code style and bogus code everywhere, and it is even hard divide the patches in functional pieces
for different issues that allow functional validation of each one.
I will work a better patch for this now that I know that you agree with this changes.
Probably it could be a right road-map, to get first a functional version in alpha or beta, and then start working in the re-styling or
recoding of some parts of the module.
#3
There are some long delays between CVS committing and dev releases, that is because there were some code that was already committed.
I am not using CVS for contrib devel modules (usually only dev releases), because as I am working with svn, and migrating to git, adding the cvs to my workflow could be really a PITA.
About last comment, watch up that these function is a theme preprocess function of a form not a hook_form function.
I agree that there are many code re-organization tasks that could be performed.
Please, if you could indicate what issues or tasks are required to release a stable versions, we could try to help moving this module forward.
#4
This looks pretty clean to me.
I'd prefer delaying commit for someone else to review it also and confirm it.
Since currently we're focussing on bugfixes only i'm perfectly OK in doing this in the 1.x branch (dev).
My most concerning issue is the ahah/ajax implementation.
I've checked the code (and added some improvements) and it IS very ugly.
Since ahah has also many issues in drupal and some codes are very inefficient i'm thinking about adding ctools as a dependency and use those tools. That's where we could use help very well.
Looking forward to commit this ASAP.
#5
Since i want to keep pushing things forward i've reviewed it again, found no issues with it and committed it unchanged.
Looking forward to see more cleanups. :-)
#6
The template file advpoll-display-binary-form.tpl.php is lost in the commit.
#7
Added the file. Forgot to cvs add... Thanks!
#8
Automatically closed -- issue fixed for 2 weeks with no activity.
#9
The "advpoll-display-binary-form.tpl.php" file will be taken into account when it is residing in the module's directory, but NOT when I copy it to my theme's directory! Anybody else having this issue?
#10
So is there something missing in advpoll_theme() or do we need to update the theme call?
#11
I solved the mistery.
The theme function is for theming a form with the name: advpoll_voting_binary_form.
But the template file in the theme definition is called: advpoll-display-binary-form
So when you override in your theme it should be called with the name of the form theme function: advpoll_voting_binary_form.tpl.php
or you have to redefine the theme function in your theme with a new template filename.
I would recommend unifying the template name with the theme function names in the module, because it doesn't have sense.
But this should be a new issue.
#12
Good to know what this problem caused. Could you maybe attach a patch? Thanks.
#13
The patch is that you have to override the theme function in your theme with a template file called advpoll_voting_binary_form.tpl.php
There is no bug, the Drupal override theming system uses the theme function not the defined template file name.
A new feature request it would be to change the name of the existing template file to avoid confusion.
#14
It is Drupal standard that a module provided *.tpl.php file can be copied *as is* into the theme directory, without the need to rename it to something else. Don't you agree this should be fixed within the module?
#15
Well, the Drupal theme functions allow defining a template with whatever filename, but the default one is with the same name.
So strictly speaking, there is no really any bug/error in the code, probably just an unusual or non-standard use of that functions.
In any case, and as I don't use to complain against people that share their time developing modules for the community, I decided to improve this module opening this feature request 11 hours ago #879760: Change template filename to match theme function name.
You can test it or look for the developer that created that function and stand hammering his head :-), take it easy, I use to see really ugly code in modules in the same way that I find fantastic master pieces.
#16
Automatically closed -- issue fixed for 2 weeks with no activity.
#17
thanks! solved it for me too.