Timestamps in chat formats should have IDs

[aaroncampbell]

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.

[aaroncampbell]

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.

[aaroncampbell]

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.

[lancewillett]

+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?

[aaroncampbell]

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.

[johnbillion]

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?

[aaroncampbell]

Using sanitize_title_with_dashes() would be fine.

[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.

[aaroncampbell]

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.

[toscho]

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.

[toscho]

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.

[ocean90]

[24399] / #24452