Add timestamp helper #41

Merged
merged 1 commit into from Mar 7, 2013

Projects

None yet

6 participants

@calebthompson
Contributor

No description provided.

@jferris jferris and 1 other commented on an outdated diff Feb 20, 2013
spec/helpers/timestamp_helper_spec.rb
@@ -0,0 +1,20 @@
+require 'spec_helper'
+require 'nokogiri'
@jferris
jferris Feb 20, 2013 Member

Why do we need nokogiri here?

@calebthompson
calebthompson Feb 20, 2013 Contributor

No we don't.

@jferris
Member
jferris commented Feb 20, 2013

Is this extracted from an application? Can you show how it was used? I'm trying to gauge how generally useful such a specific helper can be.

@calebthompson
Contributor

Sure. We're using timestamp whenever we need to show when something was created in the UI. In this example, we add a timestamp to comments:

<%= content_tag_for(:li, comment) do %>
  <%= gravatar_image_tag(comment.user_email) %>

  <div class="comment-wrapper">
    <header>
      <h4><%= link_to(comment.user_name, comment.user) %></h4>
      <%= timestamp(comment.created_at) %>
    </header>
    <section>
      <p><%= comment.body %></p>
    </section>
  </div>
<% end %>
@calebthompson
Contributor

Also, all of the comments here on GitHub seem to use a similar structure, albeit with the addition of javascript to build up the relative dates (link tag with relative date text and full date title attribute)

@mjankowski
Member

I found this thing yesterday: https://github.com/jgraichen/rails-timeago

@gylaz gylaz commented on an outdated diff Feb 20, 2013
lib/flutie/timestamp_helper.rb
@@ -0,0 +1,8 @@
+module TimestampHelper
+ def timestamp(time, options = {})
+ options[:tag] ||= :p
@gylaz
gylaz Feb 20, 2013 Member

Wouldn't this modify the original options hash. It could cause an unwanted behavior. How about:

  tag = options.fetch(:tag, :p)
  content_tag(tag, :title => l(time)) ...
@calebthompson
Contributor

I can kind of see the appeal, but if you don't care about the updates then just using Rails' time_ago_in_words avoids having to add a jquery plugin.

@mjankowski
Member

I also think showing a before/after is better than showing in-context...

    Before:

    <p title="<%= @post.created_at %>">
      <%= time_ago_in_words @post.created_at %>
    </p>

    After:

    <%= timestamp @post.created_at %>
    => <p title="February 15, 2013">5 days ago</p>

    Options:

    <%= timestamp @post.created_at, tag: :span %>
    => <span title="February 15, 2013">5 days ago</span>
@jferris
Member
jferris commented Feb 21, 2013

The before/after definitely helps show the value of this method. I'd like to get a designer's take on the markup and usage.

cc @kylefiedler @plapier @frechg @ehmorris @kaishin @Magnus-G @alexbaldwin

@ehmorris
Contributor

Having the title attribute on by default seems weird to me. I wouldn't want to prevent people from doing that, but this title value that we're putting in seems more like an alternate value than an expansion.

http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the-title-attribute

Also, see that some elements are more strict about how their title attributes are supposed to be used, since we're allowing any tag to be used: http://www.whatwg.org/specs/web-apps/current-work/multipage/text-level-semantics.html#the-abbr-element

@plapier
plapier commented Feb 21, 2013

I think by default this should be using the html5 time element with the datetime attribute.

Does the @post.created_at conform with the html5 datetime attribute?

http://www.w3schools.com/tags/att_time_datetime.asp

@calebthompson
Contributor

I looked into time, but that specifically disallows relative times.

Also, the purpose of the title attribute, for my money, is to provide a full-date but still human-readable version of the timestamp for when users want to see what that is. I think that screen readers, which will either get both values or only the title, will be able to deal with this just fine.

@calebthompson
Contributor

This is incidentally similar to what timeago is doing.

@jferris
Member
jferris commented Feb 22, 2013

@calebthompson can you explain what you mean about not allowing relative times?

According to the spec, it seems like you can put anything you want inside the element as long as you specify a valid datetime attribute.

@calebthompson
Contributor

@jferris, I looked at MDN and found this description, which is what I based my statement on (emphasis added):

The HTML time element (<time>) represents either time on a 24-hour clock or a precise date in the Gregorian calendar (with optional time and timezone information).

This element is intended to be used presenting dates and times in a machine readable format. This can be helpful for user agents to offer any event scheduling for user's calendar.

@jferris
Member
jferris commented Feb 22, 2013

There's more detail here: http://www.whatwg.org/specs/web-apps/current-work/#the-time-element

It includes this snippet:

The machine-readable equivalent of the element's contents must be obtained from the element's datetime value

Based on the three specifications I've read, I'm confident that we can put any text we want inside the tag (including relative times) as long as we have a valid, machine-readable datetime attribute.

@calebthompson
Contributor

Excellent. I think that is preferable in any case.

On Feb 22, 2013, at 3:27 PM, Joe Ferris notifications@github.com wrote:

There's more detail here: http://www.whatwg.org/specs/web-apps/current-work/#the-time-element

It includes this snippet:

The machine-readable equivalent of the element's contents must be obtained from the element's datetime value

Based on the three specifications I've read, I'm confident that we can put any text we want inside the tag (including relative times) as long as we have a valid, machine-readable datetime attribute.


Reply to this email directly or view it on GitHub.

@calebthompson
Contributor

I've changed the default tag to <time> and am offering the option of skipping the title attribute.

More thoughts?

@jferris @plapier @ehmorris @mjankowski

@jferris jferris commented on an outdated diff Mar 7, 2013
lib/flutie/timestamp_helper.rb
@@ -0,0 +1,10 @@
+module TimestampHelper
+ def timestamp(time, options = {})
+ tag = options.delete(:tag) { :time }
@jferris
jferris Mar 7, 2013 Member

@calebthompson I think @gylaz mentioned this earlier, but you're mutating the options parameter here. That can lead to some pretty confusing side effects.

@jferris jferris and 1 other commented on an outdated diff Mar 7, 2013
lib/flutie/timestamp_helper.rb
@@ -0,0 +1,10 @@
+module TimestampHelper
+ def timestamp(time, options = {})
+ tag = options.delete(:tag) { :time }
+ options[:title] = options[:title] == false ? nil : l(time)
+ options[:datetime] = time.to_s
+ content_tag(tag, options) do
+ "#{time_ago_in_words(time).capitalize} ago"
@jferris
jferris Mar 7, 2013 Member

Worth moving this into I18n?

@jferris
jferris Mar 7, 2013 Member

Also, probably simpler to use the 3-argument format instead of providing a block.

@calebthompson
calebthompson Mar 7, 2013 Contributor

I don't think I'd add translations for "ago", and I'm not seeing translations currently. Is that correct? I'd be more comfortable adding another option.

@jferris
jferris Mar 7, 2013 Member

Thinking about it some more, let's leave it inline and worry about making it customizable if we ever need to change it.

@calebthompson
calebthompson Mar 7, 2013 Contributor

I like it.

@jferris jferris commented on an outdated diff Mar 7, 2013
lib/flutie/timestamp_helper.rb
@@ -0,0 +1,9 @@
+module TimestampHelper
+ def timestamp(time, options = {})
+ options = options.dup
+ tag = options.delete(:tag) { :time }
+ options[:title] = options[:title] == false ? nil : l(time)
+ options[:datetime] = time.to_s
+ content_tag(tag, options, "#{time_ago_in_words(time).capitalize} ago")
@jferris
jferris Mar 7, 2013 Member

You have a whoopsie: options go at the end.

@calebthompson calebthompson Add timestamp helper
* Use <time> as default tag
* title: false skips title
* Don't mutate options hash
dc9f696
@calebthompson calebthompson merged commit dc9f696 into master Mar 7, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment