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

added deserialization of Date attributes for custom elements #122

Merged
merged 2 commits into from
May 8, 2013

Conversation

bsatrom
Copy link
Contributor

@bsatrom bsatrom commented May 8, 2013

Here's my first pass at adding Date deserialization support to attrs.js, as per #119 (tests included). Let me know what you think, and/or if I need to make any additional changes.

@ghost ghost assigned sjmiles May 8, 2013
@sjmiles
Copy link
Contributor

sjmiles commented May 8, 2013

Thanks for working on this.

I think we want to avoid doing any parsing outside of Date.parse.

If we really want to add a new type of string date format, I suggest we do it by overriding Date.parse and not inline in attrs.js.

WDYT?

@bsatrom
Copy link
Contributor Author

bsatrom commented May 8, 2013

Do you mean to say that attrs.js should only support YYYY/MM/DD formats (as opposed to also . and -)? If so, that's okay by me. That additional logic was in there to paper over inconsistencies w/ how the Date constructor handles the latter two types of strings, anyway.

As for Date.parse, do you think we even need to do that, or will new Date(inValue) do? new Date() passes all of the specified tests, and adding Date.parse doesn't seem to add any value, unless of course i'm not testing a case where it would be needed.

If just using new Date() is fine, I'll update the PR.

@sjmiles
Copy link
Contributor

sjmiles commented May 8, 2013

If we write the Date construction roughly this way:

new Date(Date.parse(value) || Date.now())

Then attrs.js supports whatever formats Date.parse supports.

In a separate module we can opt-in to adding new formats to parse without having to ever bother attrs.js. Something like:

var originalDateParse = Date.parse;
Date.parse = function(s) {
  if (/* s is special*/) {
  // do special handling on s
  else {
    return originalDateParse(s);
  }
}

That way we can decouple Date enhancements from attribute parsing in Toolkit.

@bsatrom
Copy link
Contributor Author

bsatrom commented May 8, 2013

Thanks Scott, makes sense. I'll update the PR for the baseline case and then whip up a quick module impl for additional format support.

@sjmiles
Copy link
Contributor

sjmiles commented May 8, 2013

Great, thanks again Brandon.

@bsatrom
Copy link
Contributor Author

bsatrom commented May 8, 2013

Sure thing! Thank you for letting me jump in 👍

@sjmiles
Copy link
Contributor

sjmiles commented May 8, 2013

LGTM

sjmiles pushed a commit that referenced this pull request May 8, 2013
added deserialization of Date attributes for custom elements
@sjmiles sjmiles merged commit 55a7a2f into Polymer:master May 8, 2013
@bsatrom bsatrom deleted the date-attrs branch May 8, 2013 20:58
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.

2 participants