Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years 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 3 years ago.
24288.2.diff (1.2 KB) - added by aaroncampbell 3 years ago.
24288.3.diff (1.1 KB) - added by aaroncampbell 3 years ago.

Download all attachments as: .zip

Change History (14)

3 years ago

#1 @aaroncampbell
3 years 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.

#2 @aaroncampbell
3 years 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.

#3 @lancewillett
3 years 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?

#4 @aaroncampbell
3 years 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.

#5 @johnbillion
3 years 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?

#6 @aaroncampbell
3 years ago

Using sanitize_title_with_dashes() would be fine.

#7 follow-up: @toscho
3 years 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.

#8 in reply to: ↑ 7 @aaroncampbell
3 years 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.

#9 @toscho
3 years 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.

#10 @toscho
3 years 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.

#11 @ocean90
2 years ago

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