When working on the initial D7 port, I suggested the following feature:
Elaborations should probably be visible in the table at /relationships/requests (pending relationships page). It is important to show a receiving user the comment before he/she declines the relationship. Title this column "Comments".
Berdir responded as follows:
Agreed, but the problem is that this is more complicated than it sounds and requires quite some changes to the user_relationships_ui module. The table is built in a template file and other modules have absolutely no chance to alter this. Just like I did for userpoints.module, we should probably replace/extend these templates with renderable structures. Then we can add a simple alter hook over the while thing and other modules can do whatever they want with these tables. I suggest a single "Revamp relationship listings" follow-up issue where we can do this.
So I'm creating this issue as he requested! ;-)
--Ben
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | revamp_listings9.patch | 40.49 KB | berdir |
| #16 | revamp_listings8.patch | 41.26 KB | berdir |
| #15 | revamp_listings7.patch | 32.47 KB | berdir |
| #14 | revamp_listings6.patch | 32.48 KB | berdir |
| #13 | revamp_listings5.patch | 33.25 KB | berdir |
Comments
Comment #1
berdirOk, here is a first patch. Not perfect yet and missing documentation but a lot of under the hood changes that need to be tested.
- Dropped the two templates, using renderable arrays instead
- Alter alter hooks which allows the elaboration module to adds it's own column, dropping extra_for_display stuff
- Made the whole column and table stuff a bit more flexible, for example, the Operations columns should now only show up if you have the permissions to do any operations. Will be especially useful when we add the view all relationships permission.
Please test and report any issues, we might want to postpone some stuff to separate issues, there are many possibilities for visual improvements but we need to take one step after another :)
Comment #2
BenK commentedThe headers on the relationships table is looking good and the "Operations" column is functioning well (hidden is the user doesn't have "Maintain own relationships" permission).
A few things I noticed:
a) The "Pending" tab is using entirely different table columns. I remember you said this tab was handled differently, but now that we're revamping this, is it possible to standardize on the same columns used on the other tabs? Currently, "sent" requests and "received" requests are in separate tables on this tab. We could either add a "Sent/Received" column just for this tab or else divide this into two separate pages/tabs (one for "Sent Requests" and other for "Received Requests").
b) Should the "Relationship" column be labeled 'Relationship type"? I'm not sure.
c) Instead of "All my relationships", should the page title for the tabs just be "My relationships"?
d) This is probably for another issue, but I wonder if we should have a view page and edit page for each relationship. Facebook seems to have this now. It would give us a place to edit the elaborations/comments also. Just a thought.
Overall, this patch seemed to function fine and I didn't notice any obvious bugs.
--Ben
Comment #3
berdirSome answers, no patch yet. We should try hard to avoid solving every problem in this issue, smaller, incremental improvements and changes are easier to track and test.
a) Many different things in here...
- We can try to make it more similar, including showing the picture too and so on. We could go as far as using a single function to convert a relationship object into a table row and then use that everywhere, just like I did for UP.
- This would also allow to unify the new hooks, so the elaborations module wouldn't have to implement two different ones.
- The two tables there are confusing, agreed, especially if you have many of both which means both have a separate pager. I think I like having two separate tabs more than a single table, what do you think? This would also allow us to display the "you have no requests" inside the table, just like that's done everywhere else.
b) Not sure either. I thought about that, but imho, "Relationship type" is too technical. Just like I did for UP categories, I think that is a column that we can completely hide, if there is just a single relationship type. No point in having a column telling you: This is a friend. This one too. Yeah, this one too. :). Some problems with that idea:
- It makes the columns very dynamic, the only one that is always shown is the username, picture is configurable, type can be hidden. Currently, elaborations module always shows it's comments after the relationship type column, that won't work anymore. One way to solve this would be to use the "Privatemsg approach" where we allow to configure all displayed columns and their columns. Another way would be use the views, this would mean that views would be a required dependency, I think the commerce project is doing that too. This would also enforce good views integration, a big problem in 6.x-1.x when you look at the issue queue. Thoughts?
- Not sure what do with one way reciprocal relationships. Probably fall back to showing the relationship type and maybe even two local tasks ( if we keep them, but we already have an issue to discuss *that* ;))
c) Makes sense...
d) Yeah, certainly another issue. Two, actually. One for editing relationships and the other for trying out what to do with these elaborations. A textfield that is shared between the two users is just weird :). Thinking about that, the only thing that *can* be edited in a relationship is that elaboration (You can't change the type nor user after creating a relationship..), so maybe the elaborations module should provide a separate edit form just for that? So we don't want an issue to edit relationships but simply comments. Of course, once relationships are an entity and you can for example attach images to a relationship, then all's different again :) But that's 7.x-2.x stuff
Comment #4
BenK commentedSome thoughts:
a) Yes, I'd be in support of "using a single function to convert a relationship object into a table row and then use that everywhere, just like I did for UP." I just think less code to maintain is probably better and when you've have all of those tabs, it's weird to have one that isn't the same. We'd be better off standardizing on a single function. Yes, I also prefer the separate tabs approach with a tab for "Sent requests" and another for "Received requests." So it seems like we're in agreement on that one.
b) It sounds really complicated to hide that relationship type column. My vote would be not to worry about that for now and just display the column. We'll be moving in the Views direction in the 7.x-2.x branch anyway, so why create extra work now that may just be removed later? And yes, you're right, probably just "Relationship" as the header would be fine. As for how to list reciprocal relationships, we might be able to solve that better after we deal with revamping relationship names. For instance, we might be able to list it for a "follow" type as "follower and following" or something like that.
d) Okay, we can postpone this discussion. One thought would be if each user has their own comments field with an option to display it to the other party.
--Ben
Comment #5
berdirNew patch..
a) Implemented as discussed.
c) Changed. We probably want different titles for sent/received pages, I haven't updated these yet
When testing, make sure that the order is always the same and always correct, with/without user pictures and with/without edit permissions.
Comment #6
berdirUpdated patch that fixes some notices...
Comment #7
BenK commentedWhen visiting the "Pending" requests page of another user, I'm getting the following notice:
I tested the latest patch with various configuration options and permission levels. The order of columns is very consistent. Here's what I noticed:
a) I'm getting this warning when visiting another user's "Pending" relationships tab:
Warning: Illegal offset type in user_relationships_pending_requests_page() (line 107 of /Users/benkaplan/git/drupal-7.0/sites/all/modules/user_relationships/user_relationships_ui/user_relationships_ui.pages.inc).
b) Should the "user/3/relationships/requests" page also be separated into sent and received (when viewing another user's pending requests)?
c) Is there any way to display the number of pending sent or received requests in the tab (i.e. "Sent requests (3)" or something similar)?
d) An idea: Change "All" to "Current". Then put "Received requests" and "Sent requests" as sub-tabs underneath one common "Pending" tab. What do you think?
e) Yes, I'd change the title on the Sent and Received pages.
f) It seems that operations links are being displayed with backwards logic. If you sent a request, it is showing 'Approve' and 'Decline' links, but if you received a request, it is showing the 'Cancel' link. This should be the opposite.
--Ben
Comment #8
berdirb) Yes, sure, will do that.
c) Yes, that is possible, but maybe do that in #1102472: Make local tasks on /relationships pages configurable?. We only deal with the sent/received tabs here because that affects the listing directly.. Or do the opposite, merge the other issue in here, since it's so heavily related (that issue would add a setting that also affects the listings...) What do you think?
d) See above.
e) To what? :) My/Someone's sent/received requests? Thinking about that, "My received requests" sounds kinda strange to me.
f) Will fix that.
Comment #9
BenK commentedc) Yeah, I think it would be could to merge the other issue in with this one. This seems like the umbrella issue and that other one is a subset. Since they overlap so much makes sense to work in one place.
e) How about something like:
"My relationships (pending sent requests)"
"My relationships (pending received requests)"
--Ben
Comment #10
berdira) Should be fixed now.
b) Changed.
c) Implemented. I like it.
d) Not sure I like that, you won't see the indicators added in c) unless you click on on Pending for example, kinda pointless. I *did* change All to Current, however.
e) Implemented, not sure if I like it. It's quite long. Maybe we can make a single "thing" ( not a title with subtitle). Something along the lines of "My pending sent relationship requests" maybe? Or without "relationship"? Still really long..
Also implemented the settings from #1102472: Make local tasks on /relationships pages configurable.
Comment #11
berdirComment #12
BenK commentedJust tested the latest patch... nice work! We're very close to being ready to commit.
a) Confirmed this is fixed.
b) Confirmed this is fixed.
c) Looks great! I really like this.
d) Yes, you're right. You've convinced me! ;-)
e) I agree that it's too long. How about this:
"My relationships (sent requests)"
"My relationships (received requests)"
I think the "pending" part is understood.
Two new things:
f) Probably should change the default text when there is nothing listed under Received requests and Sent requests. How about make them more specific to each case as follows:
You have not received any relationship requests that are currently pending.
You have not sent any relationship requests that are currently pending.
Do we want to modify these if you are looking at another users pending? It could use the Username and "has" instead of "You have" if you're viewing another user's tab. Obviously a very minor thing.
g) The available operations are still backwards for sender and receiver. The sender sees the "approve/decline" links, while the receiver sees the "cancel" link. Were we going to fix this on another thread? I couldn't remember.
--Ben
Comment #13
berdire) Changed, better
f) Yeah, changed
g) No, I just forgot about that. Fixed now.
Comment #14
berdirNew patch, accidently added some wrong code...
Comment #15
berdirAnd another one, keeping the testbot busy :)
Comment #16
berdirOk, new patch, updates old relationships/requests links and the pending text including an update function.
Comment #17
berdirComment #18
BenK commentedLatest patch looks good... this is RTBC.
--Ben
Comment #19
berdirCommited.