Here is a first stab at posting voting forms with ajax. The advantage should be obvious, especially on pages listing many forms.
It's just implemented for binary voting in this patch, but if we feel it's done right, it shouldn't be hard to add it for rankings as well.
I'll add the javascript file below.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | advpoll_ajaxvote_1.patch | 9.58 KB | ChrisKennedy |
| #7 | advpoll_ajaxvote_0.patch | 9.57 KB | ChrisKennedy |
| #6 | advpoll_ajaxvote.patch | 7.62 KB | ChrisKennedy |
| #2 | advpoll_binary_ajax_0.patch | 3.51 KB | anders.fajerson |
| #1 | jquery.form.js.txt | 2.65 KB | anders.fajerson |
Comments
Comment #1
anders.fajerson commentedAnd the javascript, jQuery form plugin and the advpoll specific code. We might want to split these into seperate files.
Comment #2
anders.fajerson commentedThis should work better...
Comment #3
anders.fajerson commentedThe
$ajax_outputrelated code needs to be looked over, this might not be the best way to get the result. Logic for checking if the user are allowed to view the result (permission, view settings or closed) also has to be added.Comment #4
ChrisKennedy commentedBah I had a 95% complete response and I somehow lost it by clicking a link! I'll see if I can recreate it.
I finally got a chance to check out the patch and the response time is amazing :) Sorry for taking so long.
I would move the drupal_add_js into advpoll_menu's !$may_cache section (per http://api.drupal.org/api/HEAD/function/hook_menu). If the user can't access a voting form $("form.advpoll-vote") just won't return anything. This also needs to be done for the js/css in advpoll_form(), but that's a separate issue.
I think advpoll-specific code in jquery.form.js ($(document).ready(...)) should be moved to advpoll.js, which will makes more sense organizationally and will simplify maintenance of the external plugin.
For the binary.inc changes, what about just using advpoll_view_results rather than the binary-specific function? It looks like it could then be abstracted out into an _advpoll_register_vote() function that is shared across the modes.
If we're posting a vote using Ajax we should probably cancel votes that way too.
Woo Ajax and jQuery :)
Comment #5
anders.fajerson commented1. I'll have a look on how js (and CSS) files are included.
2. It's better from a developer perspective too move the advpoll specific code out of the plugin, definately. I'm not sure if the best option is to put it in advpoll.js or if we should split it in e.g. advpoll-vote.js and advpoll-node-form.js, so the user doesn't have to download the node-form javascript. I have a feeling that that code will grow as well.
3. If it's possible to use advpoll_view_results() that's much better, I don't remember now why I didn't get it to work. If you can have a look at it it would be great.
4. I also thought about canceling, I'm not shure if it's worth the extra effort to do it with AJAX though. The idea of using AJAX when voting is that you should be able to quickly vote on several items on the same page. Cancel, on the other hand, *should* be a rarely used feauture. AJAX is cool and all, but there should really be a usercase for it.
About the view vote settings, how do we respect those settings when voting with AJAX?
Comment #6
ChrisKennedy commentedHere's an update to the patch. I switched to advpoll_view_results(), factored the handling out into _advpoll_register_vote(), and modified ranking to use ajax too. I also tweaked the error code to support multiple error messages.
Still to come: moving the js/css into _menu and correctly checking permissions for viewing results.
Comment #7
ChrisKennedy commentedOkay I moved the permission checking and vote cancellation directly into the view_results function. I also fixed the erroneous use of drupal_set_message rather than form_set_error from the previous patch.
Comment #8
anders.fajerson commentedOk, this might be out of my league, but anyway:
1.
_advpoll_register_vote()sounds like it should save a vote, but all it does is to return a message.votingapi_set_vote('advpoll', $form_values['nid'], $vote);is outside that function. If it's intentional...2. ...couldn't we just pass $_POST['ajax'] in
_advpoll_register_vote($node, $_POST). And if you want to pass the form values, _advpoll_register_vote($node, $form_values, $_POST) is more obvious. Now $_POST and $form_values are mixed.3. What are your feeling about the code structure change that this introduces, is it an improvement from how it was before the AJAX thing or is it complicating things?
4. What's left to do?
Comment #9
ChrisKennedy commented1. Renamed it to _advpoll_vote_response() - any other suggestions are fine too.
2. Fixed.
3. I think it's an improvement; it makes a more sense and consolidates code.
4. The only other thing is the jquery.form.js change. My preference is to put the adpvoll code into advpoll.js and include advpoll.js on all pages. It'll only be ~5k and is cached after the first load anyway. I'd rather keep it simple than worry about optimizing for a few kilobytes (and end up increasing server requests as a side effect).
But you also have a point that the code will grow over time. If you want to separate them that's fine, as long as we move the advpoll code out of the jquery form plugin file.
Comment #10
ChrisKennedy commentedAfter mucking around with the javascript I realized that your way made more sense, so I split the js into advpoll-vote.js and advpoll-form.js as originally suggested. It's committed, woo! (a rather far-reaching one too) http://drupal.org/cvs?commit=46635
Comment #11
(not verified) commented