-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
resolves bug on days when DST is added. #2009
Conversation
@vsn4ik Looks good to me, what do you think? |
Can't reproduce issue. I can not find a city with AEDT. |
@vsn4ik are you on Mac? Try Point Cook, Australia. It is AEST and on October 2nd, it switches to AEDT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I reproduced the problem. Complicated decision, but the best I can not offer.
@@ -528,7 +528,20 @@ | |||
}, | |||
|
|||
_utc_to_local: function(utc){ | |||
return utc && new Date(utc.getTime() + (utc.getTimezoneOffset()*60000)); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excess line.
return utc && new Date(utc.getTime() + (utc.getTimezoneOffset()*60000)); | ||
|
||
if (!utc) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
move to the previous line.
var local = new Date(utc.getTime() + (utc.getTimezoneOffset() * 60000)); | ||
|
||
if (local.getTimezoneOffset() !== utc.getTimezoneOffset()) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
move to the previous line.
local = new Date(utc.getTime() + (local.getTimezoneOffset() * 60000)); | ||
} | ||
|
||
return utc && local; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simply return local;
@vsn4ik I'm not sure I understand what you mean by
Are you saying you accept the pull request with the suggested changes? Do you want me to submit the changes? |
@kylethielk LGTM! It is necessary to correct the remarks. |
@kylethielk can you fix the comments from @vsn4ik. After those fixes the PR is ready! |
Thanks @kylethielk! |
…e for bootstraplib theming Note also that the 000 patch is no longer relevant as 1.9.0 includes the same fix uxsolutions/bootstrap-datepicker#2009
…e for bootstraplib theming Note also that the 000 patch is no longer relevant as 1.9.0 includes the same fix uxsolutions/bootstrap-datepicker#2009
* upgrade bootstrap-datepicker from 1.6.4 to 1.9.0; setup infrastructure for bootstraplib theming Note also that the 000 patch is no longer relevant as 1.9.0 includes the same fix uxsolutions/bootstrap-datepicker#2009 * Patch sass code for BS4 support and more general color contrasting * Wrap sass compilation into reusable function * remove check warning * Have bootstrapPage() use bootstraplib * yarn build * Use new output_template() * Deprecate bootstrapLib() in favor of bootstraplib::bootstrap() * Require bootstraplib 0.1.0.9001 * Sync up DESCRIPTION * document * rollback changes to pkgdown
On days when DST is added this results in getDate call returning the incorrect day.
As an example on October 2, 2016 many parts of Australia move from AEST to AEDT. Clicking on October 2, 2016 and then triggering datepicker('getDate') results in October 1, 2016 being returned.
Here is a JSFiddle demonstrating the issue (with instructions): https://jsfiddle.net/kylethielk/x6eafu3g/
Here is a JSFiddle with my proposed changes that resolve the issue:
https://jsfiddle.net/kylethielk/aea7jobw/2/
Let me know if you have any questions.