Followup from #607244: Decrease performance impact of contextual links. Edit and delete are duplicates, approve just shouldn't be there unless the comment is unpublished.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
2.21 KB

doh.

catch’s picture

Screenshots:

comment_links_before.png

comment_links_after.png

David_Rothstein’s picture

Issue tags: +Contextual links

I'm intending to add some more detailed comments to #607244: Decrease performance impact of contextual links about this later today, but it's not entirely clear that comment links should be contextual links at all (for both performance and usability reasons). They kind of seem to have slipped into the contextual-links patch at the last minute, but IMO shouldn't have because comments are a special case and really need their own issue.

So the best way to fix this might turn out to be different than the patch here...

Bojhan’s picture

Issue tags: -Contextual links

This seems somewhat bigger implications than this patch implies. Because how will we handle, report spam links? How will a themer work on that, does he have to diffrentiate between diffrent permission action links?

catch’s picture

FileSize
1.64 KB

Spoke to Bojhan in irc. For now, we should just rollback contextual links for comments. Then we can add them back once this usability issue, and the performance issue over at #611590: Statically cache comments are both resolved.

catch’s picture

Title: Duplicate administrative links on comments » Roll back comment contextual links (temporarily)
Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Seems good to me, I don't see us adding contextual links to comments soon - since we have many unresolved UX issues. I hope tha_sun can chime in, because I thought he removed it, and also put it in.

Dries’s picture

Before rolling this back, I'd like to discuss this some more in #607244: Decrease performance impact of contextual links. Still waiting for feedback on my last comment there. I'm not against rolling this back if necessary, but we don't need to rush it. Let's discuss it first.

sun’s picture

Issue tags: +Contextual links

.

Crell’s picture

@Dries: There's another problem with removing the $links versions: From a theming perspective, contextual links may not always be desirable. If they're not available, we need the functionality to still be available in the current mechanism. See: #616618: Allow contextual links to be disabled

IMO it's way too soon to move critical functionality to what is a brand new and untested UI concept, especially this late in the dev cycle. Let me mature first and see how much of an impact it has on theming and other workflows.

sun’s picture

Status: Reviewed & tested by the community » Closed (duplicate)