Problem/Motivation

Breadcrumbs on the comment edit and comment delete page includes 'comment-permalink' which is nuffy.

Proposed resolution

Build a proper title callback for comment canonical route
Display comment parent in breadcrumbs is any

Remaining tasks

Patch
Review

User interface changes

Change to breadcrumbs to

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because busted UX
Issue priority Normal because breadcrumbs are meh
Prioritized changes The main goal of this issue is usability. This is a prioritized change for the beta phase.
Disruption 3/5s of stuff-all

CommentFileSizeAuthor
#18 Screenshot 2015-02-10 17.19.33.png42.86 KBlarowlan
#17 2398075-comment-breadcrumb-17.patch6.44 KBandypost
#17 interdiff.txt7.28 KBandypost
#16 2398075-comment-breadcrumb-16.patch5.08 KBandypost
#16 interdiff.txt4.33 KBandypost
#14 c_delete.png21.69 KBandypost
#14 c_edit.png17.9 KBandypost
#14 c_reply_pid.png18.41 KBandypost
#14 c_reply.png10.22 KBandypost
#14 2398075-comment-breadcrumb-14.patch4.89 KBandypost
#14 interdiff.txt5.24 KBandypost
#10 Comment-reply.png61.96 KBsarav.din33
#10 Delete-comment.png59.88 KBsarav.din33
#10 Comment-edit.png67.38 KBsarav.din33
#9 2398075-4-9.txt2.38 KBnemethf
#9 breadcrumbs_for-2398075-9.patch3.24 KBnemethf
#5 comment-edit-breadcrumb.png21.51 KBhussainweb
#4 breadcrumbs_for-2398075-4.patch1.39 KBhussainweb
Screenshot 2014-12-24 08.23.03.png19.61 KBlarowlan

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: Breadcrumns for comment-editing are pants » Breadcrumbs for comment-editing are pants
dawehner’s picture

dawehner
Primary language German
commented

Isn't the problem "just" that we choose a bad title and not rely on the comment label by default?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Novice

Yes

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new breadcrumbs_for-2398075-4.patch1.39 KB

Initial attempt. Do you think this needs tests?

hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new comment-edit-breadcrumb.png21.51 KB

Here is a screenshot with the patch.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Yes please to test, can be phpunit if preferred.
Also I think changing route title would also resolve this

idebr’s picture

idebr commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Settings to 'Needs work' per #6

nemethf’s picture

nemethf commented
Issue summary: View changes

The strange breadcrumb link is available on the comment delete confirmation page as well.

nemethf’s picture

nemethf commented
Status: Needs work » Needs review
StatusFileSize
new breadcrumbs_for-2398075-9.patch3.24 KB
new 2398075-4-9.txt2.38 KB

I added a plus condition for comment delete route on the same way as in #4. Additionally the patch contains 3 tests for breadcrumb on comment pages:

  • comment edit
  • comment delete confirm
  • comment reply
sarav.din33’s picture

sarav.din33 commented
Status: Needs review » Needs work
StatusFileSize
new Comment-edit.png67.38 KB
new Delete-comment.png59.88 KB
new Comment-reply.png61.96 KB

I have tested the following three scenarios,

Checking breadcrumb links,

on comment edit (refer: Comment-edit.png) -> Showed comment title as link.
comment edit

on comment delete confirm (refer: Delete-comment.png) -> Showed comment title as link.
delete comment

on comment reply (refer: Comment-reply.png) -> Showed node title as link instead of comment title.
comment reply

Comment edit, comment delete works fine. Comment reply page breadcrumb shows node title instead of comment title. Comment reply should show the previous comment thread title.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs issue summary update

We need agreement on what to show in breadcrumbs

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I think that comment reply should show the commented entity title - which is what we're seeing here - thoughts?

andypost’s picture

andypost
he/him
Primary language Russian
commented

My pov:
1) add - commented entity link
2) reply - commented entity link + parent comment subject (could be link with #comment-num but expensive)
3) edit - commented entity link + current comment subject (could be a link as well), as node edit does now
4) delete - commented entity link + current comment subject (could be link as well)

Summary
- always Home >> Commented entity title link
- depending on action: parent or itself

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Breadcrumbs for comment-editing are pants » Breadcrumbs for comment entity
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Novice, -Needs tests, -Needs issue summary update
Parent issue: » #2371331: [META] Comment/Forum/History roadmap
StatusFileSize
new interdiff.txt5.24 KB
new 2398075-comment-breadcrumb-14.patch4.89 KB
new c_reply.png10.22 KB
new c_reply_pid.png18.41 KB
new c_edit.png17.9 KB
new c_delete.png21.69 KB

Suppose this should be like that

Fix for edit/delete is introduce proper title callback
fix for reply is display parent comment subject

Updated summary

@larowlan Do we need to separate issue to display comment parent subject

Status: Needs review » Needs work

The last submitted patch, 14: 2398075-comment-breadcrumb-14.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt4.33 KB
new 2398075-comment-breadcrumb-16.patch5.08 KB

fix tests by moving them to entity test
PS patch needs cleanup

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt7.28 KB
new 2398075-comment-breadcrumb-17.patch6.44 KB

Patch for final review, the only question for me now:
- do we need to check at least "view" access to parent comment and commented entities before display in breadcrumbs?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot 2015-02-10 17.19.33.png42.86 KB

looks heaps better to me
manual testing

Spoke with @andypost about access on irc - he confirmed that the page not found exception thrown in CommentController::getReplyForm is adequate w.r.t access - as route-level access applies for all of the routes on which the comment breadcrumb builder is displayed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 3edf291 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to issue summary.

  • alexpott committed 3edf291 on 8.0.x 
    Issue #2398075 by andypost, sarav.din33, larowlan, nemethf, hussainweb:...

Status: Fixed » Closed (fixed)

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