Needs review
Project:
Signup
Version:
6.x-1.x-dev
Component:
User interface
Priority:
Normal
Category:
Feature request
Assigned:
Reporter:
Created:
18 Jun 2007 at 10:38 UTC
Updated:
8 Jun 2011 at 18:19 UTC
Jump to comment: Most recent file
Comments
Comment #1
dwwi'm not taking new features for 5.x-1.x series. if you want this, write a patch for it (or find/pay someone else who can), but do it relative to the 5.x-2.* series (in HEAD of CVS at this point).
cheers,
-derek
Comment #2
Florian R commentedThanks! It would also be cool if it would possible for 5.x-2.x-dev :)
Comment #3
pribeh commentedAny possibility of seeing a simple one-press ajax button for authenticated users for D6?
Comment #4
eL commentedYes, it makes this module user-friendly...
Comment #5
ezra-g commentedChanging to a slightly more descriptive title.
Comment #6
coltraneSpeaking to ezra-g about this we agreed that it makes sense for this feature to be available on the node, in place of the form, and available as a Views field. I'll be working on it this week.
Comment #7
coltraneHere's a working patch for adding an option for node toggle link. On the Signup advanced settings there's a new option called "Include a one-click link" that places a Signup/Cancel Signup link on the node page instead of the form. It uses a tokenized GET request to signup the user and works if JS is not enabled.
Comment #8
dwwWhat if your signup form has multiple fields? See #208831-5: Quick Signup via URL. That's pretty closely related to this issue, no?
Comment #9
dwwAhh, briefly skimming the patch, the answer to my own question is: "either you get a simple JS toggle link, or you get a full form". Hrm. I guess that works...
I don't have time right now to test this, but the basic approach seems okay. If all you care about it a bit for if you're signed up or not (which is true for a lot of folks) a simple AJAXy toggle link is a step forward...
However, I don't see any views support for this link, which I'm *sure* is going to be the very next request as soon as this goes in. I'd be nice to just include that in the same commit if possible...
Thanks!
-Derek
Comment #10
coltrane#9, yep, working on Views integration now! I just jumped the gun and set it to CNR ...
Comment #11
coltraneAnd here it is with Views integration support. I'm not clear if I'm getting at the node properties in the View correctly, I don't really understand why the aliases aren't working.
The toggle interaction probably needs more feedback, perhaps like Flag module. Right now the text goes from "Signup" to "Cancel signup" so it's not super clear. The link is themeable so that someone could make it into a button if they like.
Comment #12
coltrane#11 wasn't rolled correctly.
Comment #13
ezra-g commentedThis seems like awesome work overall - I am about to do a functional test.
Minor nitpicks:
Can we move $url into the conditional where it is used?
Is there a particular reason we using an a tag instead of the l() function?
I'm going to functional test, but aside from that this looks great!
Comment #14
coltraneThanks ezra-g.
This patch moves the $url inside. It was outside because it was used in both the if and the else condition. But so was the token and the url() call, so that all could be outside the conditional or it can be duplicated, which is a minor duplication.
Yes, because it's a theme function and $signup_url has already been ran through url(), which l() calls.
Comment #15
coltraneThe use case this is being built for is #1115824: Switch from Flag to Signup for personal session schedule For COD we'd want sessions to be links and the event nodes to stay a form (for use with uc_signup). To satisfy this Signup module would have to provide per-content type Signup control settings … is that going to be allowed?
Comment #16
coltraneProvide per-content type override is rather easy. Further testing is needed, and perhaps some cleanup of the form descriptions.
Comment #17
ezra-g commentedThanks for this patch. This is a great start!
A few issues:
a) With this patch, with the signup link enabled, users can cancel their signups even if they lack the permission to do so.
b) I'd like to add
to signup.css so that the link displays on its own line
c) Degraded link functionality uses GET instead of POST (and that seems OK).
The toggle link degrades to a regular link + redirect, which seems fine to me. However, I was once reminded that doing a GET request to change information is technically more appropriate for a POST request. This came into play when building a feature that needed to bypass Varnish caching, which generally has POST requests pass through the cache. However, this can work with a GET request by creating a Varnish rule for the signup link callback and spares us a more complex "degrade from link to a form" behavior. So, the current implementation seems fine to me in that regard.
d) The toggle link is placed in $node->content instead of $node->links, which is more consistent with where the form appears now but somewhat less consistent with ajax toggle links in Drupal in general. Overall I'm fine with that.
Comment #18
ezra-g commented> d) The toggle link is placed in $node->content instead of $node->links, which is more consistent with where the form appears now but somewhat less consistent with ajax toggle links in Drupal in general. Overall I'm fine with that.
After further thought, I think $node->links is probably the right place to put these links. That's more consistent with the placement of toggle links in general and themers are accustomed to the links being in that place as well.
I've started a patch to do this.
This is a bit more complex than your average state toggle link when you factor in the situation where a user cannot cancel her own signups: She should click the toggle link and see a "You are now signed up" kind of message in place of the link, but the link should probably be a span or another non-clickable bit of text. So, that adds a bit of complexity.
Also, I think this will result in all users sharing the same token:
$token = drupal_get_token($url);Comment #19
ezra-g commentedHere's a patch that takes into account #18.
Comment #20
ezra-g commentedThis needs "Please login or register to signup" text.
Comment #21
ezra-g commentedHere's a revision that handles when signups are closed as well as the "Please login or register" text.