Make WordPress Core

Opened 23 months ago

Closed 22 months ago

Last modified 22 months ago

#24288 closed defect (bug) (invalid)

Timestamps in chat formats should have IDs

Reported by: aaroncampbell Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.6
Component: Post Formats Keywords: has-patch
Focuses: Cc:


If we add IDs to the time elements in the chat post format, and possibly wrap them in <a> tags. This would let you link to a specific line in the chat, like our IRC logs.

Attachments (3)

24288.diff (872 bytes) - added by aaroncampbell 23 months ago.
24288.2.diff (1.2 KB) - added by aaroncampbell 23 months ago.
24288.3.diff (1.1 KB) - added by aaroncampbell 23 months ago.

Download all attachments as: .zip

Change History (14)

@aaroncampbell23 months ago

comment:1 @aaroncampbell23 months ago

So the patch covers most scenarios, but if there are two items with the same timestamp we'd have duplicate IDs, which we shouldn't. Functionally it still works pretty well since there's unlikely very MANY items with the same timestamp, but it's not perfect.

@aaroncampbell23 months ago

comment:2 @aaroncampbell23 months ago

24288.2.diff ensures that there are no duplicate ids (assuming that the chat is in order by timestamp), and uses a md5 hash for the id instead of sanitizing the time with dashes.

comment:3 @lancewillett23 months ago

  • Keywords has-patch added

+1 — this could be really useful for *long* chat transcripts. But, I don't think we need the extra anchor element, the ID on time element should be good enough, eh?

comment:4 @aaroncampbell23 months ago

At first I just put the ID on the time element, but then you need to know to inspect element or view source, grab the ID, and put it in the URL. With the anchor linking to that ID, you just click it and copy the URL from your browser (after the snap-to happens, which is a good indicator to people) or right click and copy the URL.

comment:5 @johnbillion23 months ago

I'm not sure what purpose the MD5 hash serves. If I have a chat log with 200 lines in it, the page will call md5() 200 times. Ouch. What's wrong with using a sanitised date string?

comment:6 @aaroncampbell23 months ago

Using sanitize_title_with_dashes() would be fine.

comment:7 follow-up: @toscho23 months ago

  • Cc info@… added

All those links will now be part of the tab order. This would make keyboard usage very hard. So either take it out of the tab order or add a skip link before the transcript.

@aaroncampbell23 months ago

comment:8 in reply to: ↑ 7 @aaroncampbell23 months ago

Replying to toscho:

All those links will now be part of the tab order. This would make keyboard usage very hard. So either take it out of the tab order or add a skip link before the transcript.

Good point. 24288.3.diff uses sanitize_title_with_dashes() instead of md5 and also removes the links from the tab order.

comment:9 @toscho23 months ago

Looks good.

if ( $timeslug == $prev_timeslug ) should use strict comparison: if ( $timeslug === $prev_timeslug ). Slightly faster.

I am not so sure about the deep nesting level. Nested foreach constructs are usually a cry for separate functions. :)

The very long line with sprintf() would be more readable broken down into separate lines:

$time = sprintf( 
	'<time class="chat-timestamp" id="%1$s"><a href="#%1$s" tabindex="-1">%2$s</a></time>', 
	esc_attr( 'time_' . $timeslug ), 
	esc_html( $row['time'] ) 

Not part of your patch, but could be fixed here too: the white space in </cite>: </dt> should be removed.
By default dt is a block element, and the white space will be removed by the parser. When dt and dd are rendered as inline elements, the space should be controlled by CSS; extra white space is rather difficult to handle then. We had this problem with the comment form, if I remember that correctly.

comment:10 @toscho23 months ago

Is there any reason why get_content_chat() is not a child of Walker? Looks like an ideal use case, and I cannot come up with a single reason against it. A separate class would be easier to extend without too much duplication.

comment:11 @ocean9022 months ago

  • Milestone 3.6 deleted
  • Resolution set to invalid
  • Status changed from new to closed
Last edited 22 months ago by ocean90 (previous) (diff)
Note: See TracTickets for help on using tickets.