Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add datetime attribute alternative following W3C recommendations #91

Merged
merged 1 commit into from Nov 10, 2016
Merged

Add datetime attribute alternative following W3C recommendations #91

merged 1 commit into from Nov 10, 2016

Conversation

RoiEXLab
Copy link
Contributor

@RoiEXLab RoiEXLab commented Nov 6, 2016

Picky, I know.
In your README.md you mentioned the possible use of a data-timeago attribute instead of the "W3C-illegal" datetime attribute (see W3C Recommendation) which we couldn't actually use before this PR.
This PR adds the possibility to use the data-timeago attribute for the automatic rendering while maintaining backwards compatibility.

@RoiEXLab RoiEXLab changed the title Add alternative following W3C recommendations Add datetime attribute alternative following W3C recommendations Nov 6, 2016
@hustcc
Copy link
Owner

hustcc commented Nov 6, 2016

sorry for confused, that is a typo of readme. Maybe you can pr for this.

after 2.x.x, data-timeago was not supported, but 1.x.x supports.

@RoiEXLab
Copy link
Contributor Author

RoiEXLab commented Nov 6, 2016

This is a PR^^

@RoiEXLab
Copy link
Contributor Author

RoiEXLab commented Nov 7, 2016

A small question btw.: Is there any difference when using document.getElemensByClassName('yourclass') instead of document.querySelectorAll('.yourclass') ?

@RoiEXLab
Copy link
Contributor Author

RoiEXLab commented Nov 7, 2016

@RoiEXLab http://stackoverflow.com/questions/14377590/queryselector-and-queryselectorall-vs-getelementsbyclassname-and-getelementbyid

I'll treat that as a "there is a difference, but if it works it doesn't matter"

Thanks

@hustcc
Copy link
Owner

hustcc commented Nov 8, 2016

I means in v1.x.x, timeago.js supportes data-timeago attribute. But after v2.x.x, data-timeago attribute do not be supported.

The data-timeago mentioned inreadme.md is a typo. I will fixed soon.


I think there are no differences between querySelectorAll against getElementsByClassName when using timeago.js.

@RoiEXLab
Copy link
Contributor Author

RoiEXLab commented Nov 8, 2016

@hustcc I figured. But I think having a data-timeago attribute as alternative is a good idea.
The changes in this PR cause timeago.js to use the data-timeago attribute if it's available, and fallback to the current behaviour using the datetime attribute if it's not available.

@likerRr
Copy link
Collaborator

likerRr commented Nov 8, 2016

@hustcc I agree with @RoiEXLab . By accepting the PR timeago.js will become more compatible with the W3C.

Moreover i think we should leave only behavior provided by this PR, without falling back to current custom attribute datetime.

@hustcc
Copy link
Owner

hustcc commented Nov 9, 2016

ok~ compatible with jquery maybe will be perfact.

if(node.dataset.timeago) return node.dataset.timeago; // js
if(node.data) return node.data['timeago']; //jq
if (node.getAttribute) return node.getAttribute(ATTR_DATETIME); // js
if(node.attr) return node.attr(ATTR_DATETIME); //jq

@likerRr
Copy link
Collaborator

likerRr commented Nov 9, 2016

@hustcc is it a joke? 😄 We should provide compatibility with standard, not with some library until it's a plugin for the library

@hustcc
Copy link
Owner

hustcc commented Nov 9, 2016

not jquery plugin, not jquery plugin, not jquery plugin. 😄😄😄

before the PR, we use it below:

<time datetime='2016-11-09 12:12:12'></time>
timeago.render(document.getElementsByTag('time'));
// or jq selector
timeago.render($(tag));

After be compatibility with standard by data-*, then we can use it as below:

<time data-timeago='2016-11-09 12:12:12'></time>

We can not select the render javascript DOM below:

timeago.render(document.getElementsByTag('time'));

Can not be used with jquery selector as the README mentioned.

I think compatibility with W3C standard is OK, but we should keep the consistent usage.

Pool english maybe can not express what I mean -_-!!

@likerRr
Copy link
Collaborator

likerRr commented Nov 9, 2016

@hustcc thx, I understand what you mean, but let me correct you a little.

After PR you will still be able to query elements by data-* attribute in plain javascript:

document.querySelectorAll('[data-timeago]')

and in jquery:

$('[data-timeago]')

@likerRr
Copy link
Collaborator

likerRr commented Nov 9, 2016

But.. hm.. honestly I don't see the difference between:

<time datetime='2016-11-09 12:12:12'></time>
// and
<time data-timeago='2016-11-09 12:12:12'></time>

They both are queried the same way:

document.getElementsByTag('time')

@RoiEXLab
Copy link
Contributor Author

RoiEXLab commented Nov 9, 2016

if(node.dataset.timeago) return node.dataset.timeago; // js
if(node.data) return node.data['timeago']; //jq

The problem I see with this snippet, is that if jQuery is supported and the data-timeago attribute is not set the method will return an empty string without checking if the datetime attribute is set.

Nonetheless I'd like to point out that @hutscc as owner of this repo has write access to my branch - allowing her to edit my PR without me

@hustcc hustcc merged commit a6fd702 into hustcc:master Nov 10, 2016
@likerRr
Copy link
Collaborator

likerRr commented Nov 10, 2016

@hustcc @RoiEXLab nice to see it merged, good job

@RoiEXLab RoiEXLab deleted the patch-1 branch November 10, 2016 06:08
@RoiEXLab
Copy link
Contributor Author

@likerRr @hustcc thanks!

@hustcc
Copy link
Owner

hustcc commented Nov 10, 2016

😄

@RoiEXLab
Copy link
Contributor Author

@hustcc I noticed, the dist folder isn't updated yet. I'd appreciate if you could rerun minifiers etc.

@hustcc
Copy link
Owner

hustcc commented Nov 12, 2016

done, include ro locale and this pr.

hustcc pushed a commit that referenced this pull request Nov 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants