dries was complaining (rightfully so) about how inconsistent the signup UI is, both with itself, and with the rest of drupal. here's a list of quotes from him about stuff he found confusing, ugly, inconsistent, etc:

  1. the module's admin page shows two [more help] links still
  2. Open: Close Event <- what does that mean? 'Close Event' should be 'close' and it should use an operations-column with different table cells
  3. some table headers use capital letters, others use small letters only
  4. incorrect use of tabs -- tabs should be actions [he's talking about admin/signup/open vs. admin/signup/close]
  5. the UI below the body is dead ugly (lack of padding/margings, etc)
  6. 'Cancel Signup' -> Cancel signup' (we don't cap individual words)
  7. "7 individuals signed up" + table -- why don't we make that an <ol></ol> list? then the "7 individuals signed up" could be removed (you can look at the numbers), and it would be semantically better, and it would be less present -- the table attracts the eye with his background colors. or keep the table, and move the 'Cancel signup' button next to my name

...

i'm sure there'd be more if anyone took more time to look closely. ;) some of those suggestions probably warrant their own issue, so as not to make this one too huge. but, i wanted to record these ideas somewhere in case anyone is inspired to generate patches for any of this. if it's small and obvious, feel free to post it here. if it's a more major change (like ripping the tab out of admin/signup and making a single query interface), please create a new issue (though you should follow-up here with a pointer to the other issue, so folks who care about this issue will see it).

Also, I'm marking this as 4.7.0, but i'd be happy to get patches against 4.6, too, if anyone's so inclined. ;)
[note: the version number was changed to 5.x on April 21st, 2007]

thanks!
-derek

CommentFileSizeAuthor
#14 signup_ui_4.patch12.11 KBstborchert
#13 signup_ui_3.patch12.08 KBstborchert
#8 signup_ui_2.patch.badness.png129.18 KBdww
#7 signup_ui_2.patch8.36 KBstborchert
#6 signup_ui_1.patch6.46 KBstborchert
#4 signup_ui_0.patch6.47 KBstborchert
#3 signup_ui.patch3.83 KBstborchert

Comments

dww’s picture

dww
we/he/they
commented

for example, http://drupal.org/node/79332 is the issue for the admin/signup page's misuse of tabs. ;)

dww’s picture

dww
we/he/they
commented
Version: 4.7.x-1.x-dev » 5.x-2.x-dev
Assigned: Unassigned » dww

update -- as of HEAD these days:
1. is fixed
2. is fixed
3. still needs investigation
4. is fixed (via http://drupal.org/node/79332)
5-7 are still // TODO

so, 3 out of 7 are already done, and #6 is trivial (i just don't want to break a few patches i've got in the queue right now)... i'm making progress. ;)

stborchert’s picture

stborchert
he/him
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new signup_ui.patch3.83 KB

Hi.
This one fixes #6 and tries to fix #7.
I choose keep the table, and move the 'Cancel signup' button next to my name because I think it's looking better to have a table of subscribers instead of a simple list (btw: this could be done with theme('signup_subscriber_list', ...) or something like this).
I'm not really happy with the css change (!important) but its needed because the theme's style.css is overriding any setting for form (as in garland/style.css; it's really looking bad: margin-bottom: 2em).

#3 is already fixed!? I didn't find a header with small letters only.

greetings,

Stefan

stborchert’s picture

stborchert
he/him
Primary language German
commented
StatusFileSize
new signup_ui_0.patch6.47 KB

Hello again.
I've build a new patch which makes the subscribers list themeable (and get rid of this ugly css hack).
Now you can have a table of subscribers or a simple list or anything you like.

greetings,

Stefan

csc4’s picture

csc4 commented

These are great patches - any chance of a commit to help those of us without easy patchability?

stborchert’s picture

stborchert
he/him
Primary language German
commented
StatusFileSize
new signup_ui_1.patch6.46 KB

Hi.
If you'd like to, I can send you a patched version (but its not sure if it will go into signup at all).

I created a new patch (and modified it a little) to work with current HEAD.

greetings,

Stefan

stborchert’s picture

stborchert
he/him
Primary language German
commented
StatusFileSize
new signup_ui_2.patch8.36 KB

Updated the patch to work with current head.
Maybe its a good time now to bring this in before 2.3 is releases?

greetings,

Stefan

dww’s picture

dww
we/he/they
commented
Assigned: dww » Unassigned
Status: Needs review » Needs work
StatusFileSize
new signup_ui_2.patch.badness.png129.18 KB

Thanks for keeping this in the loop. I'm not opposed to applying these changes in general, and yes, we can try to clean this up before 5.x-2.3.

However:

a) After I applied the patch, cleared all the caches, etc, etc, there seem to be some php bugs, since the new table isn't getting rendered at all. See attached screenshot.

b) This patch is going to conflict with http://drupal.org/node/137911 -- and I'd rather just commit that first, since this one has to be re-rolled anyway.

stborchert’s picture

stborchert
he/him
Primary language German
commented

Argh, how awkward. I've tested the wrong installation so I didn't see this error.
Ok, I'll take signup_no_views.patch first and adapt this patch.

Stefan

dww’s picture

dww
we/he/they
commented

BTW, any reviews, testing and feedback for http://drupal.org/node/137911 would be most appreciated. ;)

dww’s picture

dww
we/he/they
commented

http://drupal.org/node/137911 is in. I'd like to get 5.x-2.3 out fairly soon. Any chance you've got time to work on a re-roll of this patch? Seems like this is the next big issue to fix, then we can turn our attention to http://drupal.org/node/137609.

Thanks,
-Derek

stborchert’s picture

stborchert
he/him
Primary language German
commented

I have to prepare a lecture for thursday but I think I can diverge a few hours. So it should be ready tomorrow, not later than wednesday.

Stefan

PS: from friday on I've two weeks of free time so http://drupal.org/node/137609 will be ready soon.

stborchert’s picture

stborchert
he/him
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new signup_ui_3.patch12.08 KB

Already finished! :-)
However, I've got one thing that could be changed eventually:
maybe its better to rename signup_form_cancel to signup_form_signup_information now?

stborchert’s picture

stborchert
he/him
Primary language German
commented
StatusFileSize
new signup_ui_4.patch12.11 KB

Argh, one minor change.

BTW: here is a mix of screenshots from the "new" ui (with different permissions set).

ivrh’s picture

ivrh commented 
#5
csc4 - July 6, 2007 - 08:50

These are great patches - any chance of a commit to help those of us without easy patchability?

Yes:
1. Drupal Pro Development book
2. PHP for Dummies book
3. 3-5 hour a day practice

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

Sorry this took a while to get to. In testing the patch, there are a number of cases where you get "warning: Invalid argument supplied for foreach() " errors due to assuming something is an array when it's not there. :( I was hoping to commit this before I started work on http://drupal.org/node/190553 since I knew they'd conflict, but this one was just not solid enough and I didn't have time to resolve everything in here immediately. So, we should probably wait to reroll this one until I commit #190553, anyway, to avoid additional work...

That said, I'd still like to fix up these problems and make signup more theme-friendly, so don't lose hope. ;)

Thanks,
-Derek

stborchert’s picture

stborchert
he/him
Primary language German
commented

so don't lose hope

Never! :-)

I'm out of time for some days but I hope to get back working on this patch on friday.
Just saw you committed #190553 so lets go and pimp up the UI ;-)

greetings,

Stefan

dww’s picture

dww
we/he/they
commented

Also, I'm not a big fan of how this patch is stuffing more and more cruft into the "signup cancel" form. honestly, the signup cancel form should just be a "cancel" button. ;) The rest isn't specifically about cancel, it's about displaying your current signup info, and/or info about other signups. I'd rather see less junk in the cancel form, since, for example, this patch produces duplicate info on the node/N/signups admin tab since we're printing the current signup info manually, and then the cancel form prints it again (i think that's what i was seeing). anyway, it'd be better if each part was it's own thing, with its own theme function, so it was easier to just show what you want, how you want it, without any display code at the logic layer or vice versa...

dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » dww

I just committed a fix for the 'Current Signups' thing to all branches:
http://drupal.org/cvs?commit=90967 (HEAD)
http://drupal.org/cvs?commit=90968 (DRUPAL-5)
http://drupal.org/cvs?commit=90969 (DRUPAL-4-7)

I'm going to take a stab at redoing the rest of this now, given the changes I made for http://drupal.org/node/190553 and my thoughts on how this should work outlined above. Stay tuned.

dww’s picture

dww
we/he/they
commented

Also fixed 'Cancel Signup' on all branches, and repaired the translations (thanks, perl -pi.bak -e 's/Cancel Signup/Cancel signup/g;' po/*). ;)

dww’s picture

dww
we/he/they
commented

@stBorchert: I guess I'm ignorant, but what good is the 'summary' attribute in the table definitions, which you included in your patches? It seems like neither FF nor Safari supports the summary attribute -- they both ignore it completely. I don't see anywhere in core that sets this attribute. Is there something I'm missing, or is this basically cruft? Thanks.

stborchert’s picture

stborchert
he/him
Primary language German
commented

I guess I'm ignorant, but what good is the 'summary' attribute in the table definitions, which you included in your patches?

Its just an addition for screen-readers so you could be informed about the content of the table without reading the content itself. Not really required but my firebug always annoys me if its not set ;-).

I didn't had the time to look through your latest changes but will do so hopefully this week.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Active

I just updated the original post by crossing out the completed items. Honestly, let's handle #5 and #7 over in http://drupal.org/node/47913. That patch is just as broken as this one, it's an earlier issue specifically about adding more theme-ability, and this issue is already getting long. Let's just definitively rule out #3 from the original list, and then mark this postponed until whatever we do in #47913 lands, and which point we can close this. Sound good?

stborchert’s picture

stborchert
he/him
Primary language German
commented

Ok, lets do so.
I've done a quick review of current head to find the headers noted in #3 but couldn't find any irregularities. Sure this one is still active?

stborchert’s picture

stborchert
he/him
Primary language German
commented
Issue summary: View changes

crossing out completed items.

dww’s picture

dww
we/he/they
commented
Status: Active » Postponed

"Sure this one is still active?"

Not at all, all I said was: "Let's just definitively rule out #3 from the original list...". Sounds like you just did, so I crossed #3 off, too. ;) Thanks!

This is hereby postponed on http://drupal.org/node/47913

dww’s picture

dww
we/he/they
commented
Status: Postponed » Fixed

Given that #47913: More themeability is now committed, I'm going to call this fixed. Sure, maybe the default UI is still a little crappy on the node itself, but a) all that junk can now be themed, b) there's a setting to move it all to a separate tab, c) there's lots of cool stuff you can do with views now.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes