Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

Make the timeago directive expand into a standard HTML5 time element #41

Merged
merged 10 commits into from
Jun 6, 2014

Conversation

gsklee
Copy link
Contributor

@gsklee gsklee commented Apr 7, 2014

This PR makes the amTimeAgo directive to always expand into a <time> element per HTML5 standards:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/time
http://html5doctor.com/the-time-element/

While there are 6 commits, the one that matters is 1c24b26.

@gsklee
Copy link
Contributor Author

gsklee commented Apr 7, 2014

Addressed a few broken tests but couldn't figure out the remaining two Error: Expected spy clearTimeout to have been called. Weird. Any ideas?

@urish
Copy link
Owner

urish commented Apr 7, 2014

@gsklee Nice to meet you and thank you for your contribution. I will review the changes shortly. Meanwhile, can you please share the motivation behind using the HTML5 time element?

@gsklee
Copy link
Contributor Author

gsklee commented Apr 8, 2014

@urish because it's the standard way to wrap any time-related value in HTML5 rather than <span> - just trying to help the web by being more semantic =)

@urish
Copy link
Owner

urish commented Apr 8, 2014

Well, yea, this makes a lot of sense. So yeah, we will start using the <time> element.

@gsklee
Copy link
Contributor Author

gsklee commented Apr 9, 2014

Cool, would be great if you have any ideas on the spyOn test issues (and why they're not spyOn().andCallThrough but spyOn().and.callThrough btw?)

@urish
Copy link
Owner

urish commented Apr 10, 2014

spyOn().andCallThrough is the old Jasmine 1.x Syntax.

Jasmine 2.0 provides a new syntax for specs, as you can read here.

@urish
Copy link
Owner

urish commented Apr 10, 2014

@gsklee I have reviewed your implementation, what is the motivation behind using ng-transclude?

@gsklee
Copy link
Contributor Author

gsklee commented Apr 11, 2014

@urish As mentioned in 0dba36b, it's added to fulfill an existing test: https://github.com/urish/angular-moment/blob/master/tests.js#L149

@gsklee
Copy link
Contributor Author

gsklee commented Apr 11, 2014

Without the transclusion the initial text will be replaced by the template completely.

@urish
Copy link
Owner

urish commented Apr 11, 2014

Thanks @gsklee, this makes sense. I'm considering a more light-weight implementation, where no additional scope is created and no transclusion is needed:

  1. If our element is already a <time> element we behave like today, but also modify the datetime attribute according to the time provided.
  2. If our element is another element (e.g. <span>) we add a new <time> element, put the text inside, and modify the datetime attribute according to the time provided.

Thoughts?

p.s. I really appreciate all the time you put into making angular-moment better :-)

@gsklee
Copy link
Contributor Author

gsklee commented Apr 13, 2014

@urish So to summarize, you'd like to see the transclusion and the isolated scope both being removed, and just wrapping the directive with <time> if it's not already there?

@urish
Copy link
Owner

urish commented Apr 13, 2014

@gsklee Exactly, you got it short and to the point!

@gsklee
Copy link
Contributor Author

gsklee commented Apr 22, 2014

@urish I've implemented condition 1; actually this is the only use case I care about. Let me know if you like it and, if it gets merged in, we'll see if it's desirable to implement condition 2 as well =)

@urish
Copy link
Owner

urish commented Apr 23, 2014

Thank you @gsklee, this looks perfect!

One last thing, it seems like you haven't merged in the changes from commit 6261c14, can you please merge them into your pull request as well?

Thanks!

@urish urish merged commit d82f255 into urish:master Jun 6, 2014
urish added a commit that referenced this pull request Jul 27, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants