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

Fix datepicker reset on early dates #1348

Merged
merged 18 commits into from
May 16, 2017

Conversation

pbugnion
Copy link
Member

@pbugnion pbugnion commented May 12, 2017

This addresses issue #1322.

Prior to this PR, all the characters were reset when a user started typing the year in the datepicker. This arose because new Date(year, ...) interprets a number below 100 as a two-digit year (i.e. 5 gets mapped to 1905), whereas the datepicker interprets it as a 4-digit year (i.e. 5 gets mapped to 5AD). Every time the user typed a character, the view forced a roundtrip to a timestamp and back to convert the date to UTC.

The fix implemented here removes that roundtrip: dates in the view and the JS model are stored as Date object without trying to force them to UTC -- there is no loss of information here since dates are time-zone aware anyway. The conversion to UTC happens just before serialization: the date gets passed to the Python layer as a UTC date.

This PR also:

  • changes the trait in the Python model to a datetime.date, rather than a datetime.datetime. The datepicker object doesn't let us set the time, so it seems a bit strange to pretend we can give that information back to the user. This change is orthogonal to the fix, so we can remove it if you don't think it's the right choice.
  • implements tests for the JS view.

What's left to do:

  • unit tests for {de,}serializers on both the JS and Python side

@pbugnion pbugnion changed the title [WIP] Fix datepicker reset on early dates Fix datepicker reset on early dates May 13, 2017
@pbugnion
Copy link
Member Author

pbugnion commented May 13, 2017

This is ready for review -- besides the bug fix, the only major change is to switch the traitlet in the widget to a datetime.date rather than a datetime.datetime. This does break backwards compatibility, so I'm happy to revert that change if need be.

@jasongrout
Copy link
Member

I tweaked things a bit and pushed to your branch. I agree with the change to using simple dates. Since we are using simple dates, though, I don't think there's any need to deal with UTC at all - just use local/naive dates. I also tweaked some of the documentation.

@jasongrout jasongrout force-pushed the fix-datepicker-tz-error branch from ac1fc21 to 148ebbb Compare May 15, 2017 19:36
@jasongrout jasongrout added this to the 7.0 milestone May 15, 2017
@jasongrout
Copy link
Member

@pbugnion - your changes look great. If my changes look good to you, let's merge. Do you approve my changes?

@jasongrout
Copy link
Member

jasongrout commented May 15, 2017

Since we are using simple dates, though, I don't think there's any need to deal with UTC at all - just use local/naive dates.

Actually, it looks like we need to normalize to UTC dates in javascript - that seems to be what the input is giving us. So that's what I did in my most recent commits here.

@pbugnion
Copy link
Member Author

Thanks for the help! I've added a test for dates before 100AD. I think this is ready for merging.

@jasongrout
Copy link
Member

Great, thanks!

@github-actions github-actions bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants