rmm5t / jquery-timeago

:clock8: The original jQuery plugin that makes it easy to support automatically updating fuzzy timestamps (e.g. "4 minutes ago").
http://timeago.yarp.com
MIT License
3.82k stars 710 forks source link

Plugin only works with TIME elements; should only require @datetime=ISO8601 #275

Closed jm3 closed 8 years ago

jm3 commented 8 years ago

Project website docs imply that the plugin will work as long as a datetime attribute is present with an ISO 8601 date, but in Chrome 51.0.2679.0 (Official Build) dev (64-bit), the plugin fails to replace the text UNLESS a <time> element is used. <span> doesn't work, for example. (Oddly, the plugin does set the @title attr, it just doesn't replace the value.) I spent a bunch of time trying to "debug" my code because I misunderstood the plugin's strong preference for <time> and <abbr> elements.

repro: http://codepen.io/jm3/pen/ZWLOoj

rmm5t commented 8 years ago

A datetime attribute is not allowed in any other HTML5 element..

The docs talk about the alternative format using the microformat syntax instead. If you put the ISO 8601 timestamp in the title attribute of any other HTML element, it should work. <abbr> tags are recommended in this case. Example:

<abbr class="timeago" title="2008-07-17T09:24:17Z">July 17, 2008</abbr>
rmm5t commented 8 years ago

Project website docs imply that the plugin will work as long as a datetime attribute is present with an ISO 8601 date

Where exactly? Here's a direct quote from the website docs that clearly explains a <time> element is required:

"...turn all <time> elements with a class of timeago and an ISO 8601 timestamp in the datetime [attribute]"

The examples are pretty exact too. The README covers this as well.

I just wasted a #$($%)(& hour debugging this.

Btw, I don't appreciate this snide quip. This library is free, and I'm glad to try and help, but please take your mistaken frustrations elsewhere.

Aside, I'm open to improving the docs if they're unclear somewhere or make a confusing implication. I just don't see where the confusion came from. Pull requests to improve docs are always welcome.

jm3 commented 8 years ago

Hi Ryan, my apologies. I was not being snide, simply frustrated after wasting some time debugging something that ended up not being a bug, but a misunderstanding. I didn't spend the additional time to open an issue to be rude; rather, I did it to encourage you to make a small change in hopes of making your project a little more more accessible in the future. Since I didn't say this previously, thanks very much for making this plugin and sharing it. I appreciate it.

I re-read the docs, and you're right: the docs only show examples for <time> and <abbr> elements. I assumed, incorrectly, that if the plugin worked for those two element types, that it would work for others. The mistake is mine, but that doesn't mean the docs are as 100.00% clear as can be :)

In my experience, plugins looking for a specific attribute or data-value don't usually need to care about the element type. I understand now that you made that choice, and it's your project so I respect that decision, but since the plugin sort of half-works on non-<time> elements (where it populates the title attribute but doesn't replace the value), this was teasingly mysterious to debug.

If it helps understand why this happened: After reading the docs, I didn't use an <abbr> tag because times aren't abbreviations, at least in my view, and I didn't use a <time> element because that sounded like a new whiz-bang HTML5 element type that I'd never encountered. I prefer to stick to core HTML elements that are broadly supported. In my (albeit limited) experience, plugins/selectors looping over elements that search for a specific attribute don't need to enforce strict HTML element policy / doctrine about element types. I've used plugins for sorting or other related things that just require your elements have the required attribute X, but that are not restrictive about which specific elements your markup uses. That "more open" approach (Postel's rule, etc.) to me seems ideal for JS plugins that end up being used in many unanticipated markup situations in the wild. It also seems a little dogmatic to say, "I want this plugin to not run if someone installs my plugin and puts a datetime attrib on, say, a <div>". I guess you believe that marking up times with non-<time> elements is… bad? I don't know. shrug

My parting POV is: if you're going to be strict about limiting the scope in which the plugin will run (i.e. only on <time> + <abbr> elements), I just think it's helpful to your users in the open source community to be extra clear about that. Something like, "this plugin will not work if your element type is anything other than X or Y". Fair?

rmm5t commented 8 years ago

@jm3 No worries. Apology accepted, but I still don't think you're fulling unerstanding things correctly.

Something like, "this plugin will not work if your element type is anything other than X or Y". Fair?

No. Not fair, because it's not accurate.

Timeago works on ALL element types. You just have to follow the specification. For time elements (new as of HTML5), you must use the datetime attribute. For ALL other elements, you must use the microformat specification which means putting the timestamp in the title attribute instead.

These aren't my specifications, and they are not unique to timeago. They are well-defined and standardized mechanisms for representing a timestamp within any HTML page. That's why timeago uses them. Timeago was invented before HTML5; that's why it still supports the older microformat syntax.

In summary, you should always use <time> to represent timestamps. However, if you're not using <time> you should be using <abbr>, because that's what the old HTML microformat spec said was best. With that said, you're still welcome to use <span> tags too, but you must use the title attribute, because a datetime attribute on a <span> would be invalid otherwise.

I hope that clarifies things some more.

jm3 commented 8 years ago

Ah OK, that's the clarification I was missing! So something like this would be correct, right:

  1. With <time> elements, the datetime attribute (containing an ISO 8601 date) is required (per WHATWG HTML5 and W3C HTML5 reco. timeago recommends this approach.
  2. If you're dealing with legacy markup or other restrictions that prohibit your use of the HTML5 <time> element with a datetime, timeago can still work for you: in this case, use of <abbr> elements is recommended (since <abbr> was previously the HTML microformats recommendation for this use). Other tags are possible but not recommended. In these cases, the ISO 8601 formatted date must be placed in a title attribute (not datetime), since datetime attributes are only valid on <time> elements in HTML5. Placing datetimes in other attributes, e.g. HTML5 data-* attributes, is unsupported.

Cool if I update my PR with language like that?

rmm5t commented 8 years ago

Cool if I update my PR with language like that?

Sure, give it a go. Adding the two clarifying paragraphs alone would be a clean PR to introduce. Please don't reword everything else in the README.

But please don't document the lack of features that don't exist. It only adds noise to the documentation. Similar to a double-negative in prose. For example:

Placing datetimes in other attributes, e.g. HTML5 data-* attributes, is unsupported.

mastef commented 5 years ago

@rmm5t I think it'd be fair to explicitely state what you have to do in case you can't use a time or abbr tag. This also cost me some time in debugging and trying to find out how to get it to work until I found this issue.

The confusing part is basically the wording : The HTML5 time tag is strongly recommended, but the legacy datetime microformat using the abbr tag is also supported: which seems to limit it to only the abbr tag.

Maybe changing it to The HTML5 time tag is strongly recommended, but the legacy datetime microformat using the abbr tag ( or any other HTML element ) is also supported: would make sense.

rmm5t commented 5 years ago

Hi @mastef. I'd gladly accept a PR with those wording changes.

mastef commented 5 years ago

Ok @rmm5t - made a PR with 2 changes for the readme.markdown and the index.html in #353

rmm5t commented 5 years ago

@mastef Looks great! Simple and to the point!

Thank you!