Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#24288 closed defect (bug) (invalid)

Timestamps in chat formats should have IDs

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

Description

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

Download all attachments as: .zip

Change History (14)

@aaroncampbell
11 years ago

#1 @aaroncampbell
11 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
11 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
11 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
11 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
11 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
11 years ago

Using sanitize_title_with_dashes() would be fine.

#7 follow-up: @toscho
11 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
11 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
11 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
11 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
11 years ago

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