Closed (fixed)
Project:
Privatemsg
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
28 Oct 2010 at 23:06 UTC
Updated:
10 Jan 2011 at 09:20 UTC
Jump to comment: Most recent file
Hey Berdir,
On the message view page, tags are currently displayed within a form field at the bottom of the page. I think it would be better to make tags a bit more prominent on this page. Here are some ideas:
1. Display tags as text links. Clicking on a tag link would take you a page of messages filtered by that tag (just as it does at /messages).
2. Display these tag links at the top of the page. The links don't need to be big. but should be somewhere near the thread title or the participants line. Possible locations include on the right side of the thread title line, on the right side of the "Between you and..." line, or on a separate line beneath either the thread title of "Between" line.
3. To help with number 2, I think we should flip the thread title and the tabs. It would make more sense if the thread title appeared near the rest of the thread (without the tabs getting in the way). So display the tabs at the very top of the page and the thread id below them.
4. Create a little red "x" next to the tag link to remove the tab. A good example of what this could look like is from the Community Tags module (http://drupal.org/node/691076). That module has a nice little "tag" icon also in the screenshot.
5. I'm wondering if the Add Tags form (currently at the bottom of the page) should really be on its own page. It's confusing right now to have that whole fieldset below all of the reply to message stuff. Are you tagging the reply or the thread? Also, it doesn't look great to have that whole fieldset beneath the "preview" and "send" buttons. It kind of makes those buttons disapper.
Instead, what about a "Add a tag" or "Tag this" link at the top of the page next to the existing tags. Clicking that link would display the Add Tags form either on the overlay or on a separate page. The thread title could also be displayed on this page for clarity's sake.
Thoughts?
--Ben
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | tags5.patch | 17.06 KB | berdir |
| #8 | tags4.patch | 17.04 KB | berdir |
| #6 | tags3.patch | 16.67 KB | berdir |
| #4 | tags2.patch | 8.9 KB | berdir |
| #1 | tags.patch | 7.16 KB | berdir |
Comments
Comment #1
berdirOk, what about this?
- Displays link on the top, above the recipients. Still needs visual improvements, but works.
- Removes the current tag alter form. Instead, a "Change" Link is displayed, when clicked, a simple form is displayed that allows to change the tags.
- Would be nice to do this with AJAX, but I'm not yet sure how that works with links.
- This also fixes a bug that makes it impossible to remove tags.
- I haven't updated the tests, wanted some feedback from you first before we continue on this path. So you can ignore the test failures for now :)
So, to your suggestions:
1. Done.
2. Done, see above.
3. Not possible. I have no control over that, what is displayed where is defined in the theme.
4. Not yet done, isn't so urgent because of the change link IMHO.
5. See above, the form is no much simpler and only displayed when requested. Should also be clear that it is for the whole thread now.
Comment #3
BenK commentedHey Berdir,
I like the general approach taken in your patch quite a lot. It's already a significant improvement. I especially like the new placement of the tags. I also like the "change" link that displays a simple version of the tagging form. Nice work!
Here are some thoughts:
A. I don't think we need commas between tags, nor do we need a period at the end of the tags. I think there can just be some extra horizontal space between each tag (similar to how taxonomy terms are displayed on nodes if you add a term reference field in D7).
B. Right now, the "Change" link looks like another tag, rather than a link. Anything we can do to make it look different? One possibility is to place it in a parentheses like this "(change)". Or should it say "(modify tags)" to be more clear? Here's how it might look:
Tags: Orange Apple Pear Peach (modify tags)
C. Should the "Tags" line go below the "Between you and ...." text, but above the horizontal line? I was thinking that maybe it's first important to know who is in the conversation, then you consider how the conversation has been tagged for further clarification. But I'm not sure about this. Might want to see how it looks. I also think there needs to be more vertical space between the "Between" text and "Tags" text (whichever order they are in).
D. When the "Tag this conversation" form is displayed, it is causing the "Between..." text to get pushed over to the right.
E. We may want to add some CSS classes to the "Tags" text and the "Between..." text so that these can be styled better.
But this is all minor stuff. The overall approach is great!
--Ben
Comment #4
berdirA/B: Change link renamed to "(modify tags)", converted tags into a inline list.
C. Moving in between is tricky, because the line is actually part of the Between-div. I would have to remove that line from the between div and add it to the tags div. Added some space, however.
D. Can't reproduce.
E. Tags: is now in a span with a class. Not sure if adding a span around the Between.. (what exactly..?) makes sense, the whole div does have a class.
Also re-added a custom alter hook to that page and updated the documentation of that. Didn't get ajax working..
Comment #5
BenK commentedHey Berdir,
I started testing the latest patch, but if I try to modify the tags any changes I make won't save. I tried both existing and new tags (as well as just deleting the current tags), but none of those changes were saved after I clicked "Tag this conversation".
Also, should there be a "Cancel" link to the right of the "Tag this conversation" button? I realize that you could just click the "Tag this conversation" button to get rid of the modify form, but if you inadvertently delete some tags or make some other mistakes, this would allow you to reset.
And maybe just try swapping the vertical order of Tags line and Between line? We could try putting Between line on top... Tags would appear beneath the horizontal line (that is part of the between div). Might be worth a test.
--Ben
Comment #6
berdirUpsie.
- Fixed the form.
- Added cancel link
- Moved the tags element below the between element but I don't really like it. Anyway, it's just one of those famous 3 lines of code to implement the hook and change the weight of it so that you can move it (almost) anywhere ;)
- Fixed and extended the tests
Comment #7
BenK commentedJust finished testing the latest patch....
* Form works great now!
* "Cancel" link works great and looks nice.
A. I agree that "Tags" line looked better above the "Between" line. Let's put it back (thanks for letting me experiment with this). If possible, add a bit more vertical space beneath "Tags" line (and above "Between" line).
B. Can we add a span class around "(modify tags)"? This way, that link can be styled differently than the tags themselves.
C. Upon saving, message currently reads: "Tagging information has been saved." Instead, how about: "Your conversation tags have been saved." or "Conversation tags have been modified." or "This conversation has been tagged."
D. If a conversation is not tagged, it currently reads: "No tags (modify tags)". Is it possible to change this to just read something like:
"(Tag this conversation)"
"Tag this conversation"
"Add a tag"
"Add a conversation tag"
"Add tags"
"(Tag this)"
"Tag this"
Those are a few of the options. I wasn't sure if this special case needed to be in a parenthesis also, but maybe not? I just thought that instead of saying there are no tags, we should just have an action link to add tags if you want to.
--Ben
Comment #8
berdirA. Moved back, added a bit more space.
B. Added a class directly on the a element, I think that should work too.
C. Using "Your conversation tags have been saved." now. Also thought about adding a special case when you delete all tags but I can't easily detect if there were any tags attached before.
D. I like "Tag this conversation". Agreed that we don't need the parenthesis.
Comment #10
berdirI need to update the patch because of the renamed links. You can test the patch anyway.
Comment #11
berdirOk, fixed the tests.
Comment #12
BenK commentedI tested the latest patch and all of your recent changes (A, B, C, and D) look and work great. I really like this a lot. This is RTBC! :-)
Comment #13
berdirGreat, commited, this can be backported to 6.x-2.x.
Comment #14
berdirPorted to 6.x-2.x