Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Quite straightforward.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | comment_new_3.patch | 2.85 KB | lilou |
| #3 | comment_new_2.patch | 2.9 KB | David_Rothstein |
| #3 | next_new_comment_link_garland.png | 6.61 KB | David_Rothstein |
| comment_new.patch | 2.19 KB | chx | |
Comments
Comment #1
catchMassive, massive +1. Tested this and the code works great. Not sure about "Next new" as text but that's minor.
This won't work across pages, because the #new links disappear once you've viewed one page request. That's a different and more fundamental issue though really.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedOoh, I like this a LOT!! I tested it and it does work great for the most part, but I noticed a few problems:
1. There's a PHP warning on nodes with no comments (easy to fix by declaring $comments_array before using it).
2. As @catch said, this will never work perfectly when there are multiple pages of comments, and that's OK -- but it's actually more broken than it needs to be. I found that if there are new comments spread across multiple pages, then the "next new" link will try to jump you to the LAST page, no matter where you are. At least it should be able to take you correctly through all the new comments on the page you're currently visiting. There seems to be something funny going on in the code with the call to comment_new_page_count() that is causing this, but I don't understand it at the moment...
3. It seems like $comments_array can be gigantic; it stores each comment, and then inside each comment is stored an entire copy of the node? I don't know anything about how the internals of PHP work, but this seems wasteful. The only new information from the node that needs to get passed along is the node comment count.
4. I agree that the text needs work. What about "jump to next new comment"? It's a little verbose, but it's good to have a verb in there, I think.
5. (feature) How about also having a "jump to first new comment" link at the top of the page? Sort of like the project issue tracker on drupal.org. This would be extremely easy to implement and would add even more to the usability.
6. (minor feature) How about putting the "jump to next new comment" link last in the list ... and perhaps theming it a bit differently from the others? For usability reasons, it might be good to separate it from the other comment links visually, since unlike the others ("delete, edit, reply") it's not something you're doing to the comment.
Sorry for the nitpicking ;) And certainly only the first 4 need to be dealt with for this to be RTBC. I'll see if I can look at this more later. Again, this is awesome! This patch might have the highest ratio of usability enhancement to lines of code in the history of Drupal ;)
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedThe attached patch fixes bugs 1 through 4 from my above post.
It seems to work pretty well now, but here's a list of extra features that could be added to make it really shine (recapping the ones listed above, plus a couple new ones). None of these are in the patch right now:
1. Add a "jump to first new comment" link at the top of the page.
2. Improve the theming. The link gets marked with class "active" (because by definition it points to the same page that we're currently on), but that's really not what we want -- it comes out an ugly black color in Garland, whereas the other links are blue. I think it might have to be overridden at the individual theme layer. What about making the color of the link the same color as the "New" mark on the comment? I've attached a screenshot of what this could look like (in Garland)...
3. If you have ten new comments all in a row, having the "next new comment" link on each of them is not very useful, and, IMO, a little annoying. So what about hiding the link except when it's needed -- i.e., only showing it when you actually have to "jump" in order to get to the next new comment? This would be simple to implement.
4. I was thinking more about getting this to work across pages, and regardless of whether #6371: All comments marked as read when viewing any page. gets fixed, the only thing the current patch would be responsible for is finding ONE new comment beyond those on the current page and making a link to it. So it's really independent of that other bug. The problem is, I can't think of a way to do this that doesn't involve a heavy hit on the database... figuring out the next new sequential comment (and also figuring out what page it will show up on!) doesn't seem so easy, given all the possible ways that comments can be sorted, etc. Any ideas?
Comment #4
lilou CreditAttribution: lilou commentedRe-roll
Comment #5
Sutharsan CreditAttribution: Sutharsan commentedMoving all usability issues to Drupal - component usability.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #7
Jooblay.net CreditAttribution: Jooblay.net commentedWhat is the status of this ticket:) Can we close this...
Comment #8
chx CreditAttribution: chx commentedComment #9
David_Rothstein CreditAttribution: David_Rothstein commentedAn oldie but a goodie. I think it's still relevant, and could even be backported to D7 (maybe) although would have to be turned off by default for existing sites.