Prevent bizarre behavior when `weekStart` is a string or other unusual value. #49

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
@todofixthis

To integrate this plugin with a PHP application, we are sending the weekStart value to the frontend as a string, e.g.:

'weekStart':  '<?php echo esc_js($report->getWeekStart('w')); ?>'

Because Javascript treats the + operator as a concatenation operator when the LHS is a string, it causes weird behavior when the datepicker computes its weekEnd setting.

This pull request changes the datepicker to implicitly converts the incoming weekStart value to an int and mods it by the number of days in the week (according to the selected locale) to prevent weird values from messing up the display of the datepicker.

@eternicode

This comment has been minimized.

Show comment Hide comment
@eternicode

eternicode Apr 22, 2012

Contributor

@linestream , I agree with your first two patches, thanks!

I don't see the need for linestream/bootstrap-datepicker@e4a0fc2e98a78f61485bde5263181c6c4d12a46b -- in what locale would the week not be 7 days? (Although, in the current system, the days array is 8 items long, "Sunday" being included twice, which it doesn't seem you took into account). Also, JS's Date.getDay "Returns the day of the week (0-6) for the specified date according to local time." So I think we can leave it hardcoded as 7.

And what's linestream/bootstrap-datepicker@e9317d5a605a0c5d2d36c77b6837d16109d9bf24 all about? It doesn't seem to apply to the ticket, and the build_standalone.less file is meant to produce a bootstrap-independent .css file (normally it would use bootstrap's icons, but in this case those should not be depended on). A stray addition to the pull request? ;)

Contributor

eternicode commented Apr 22, 2012

@linestream , I agree with your first two patches, thanks!

I don't see the need for linestream/bootstrap-datepicker@e4a0fc2e98a78f61485bde5263181c6c4d12a46b -- in what locale would the week not be 7 days? (Although, in the current system, the days array is 8 items long, "Sunday" being included twice, which it doesn't seem you took into account). Also, JS's Date.getDay "Returns the day of the week (0-6) for the specified date according to local time." So I think we can leave it hardcoded as 7.

And what's linestream/bootstrap-datepicker@e9317d5a605a0c5d2d36c77b6837d16109d9bf24 all about? It doesn't seem to apply to the ticket, and the build_standalone.less file is meant to produce a bootstrap-independent .css file (normally it would use bootstrap's icons, but in this case those should not be depended on). A stray addition to the pull request? ;)

@todofixthis

This comment has been minimized.

Show comment Hide comment
@todofixthis

todofixthis Apr 22, 2012

  • e4a0fc2 was intended to be for future-proofing. Odd that I didn't notice 8-day weeks in my testing; so it goes. As you note, it's probably overkill anyway.
  • e9317d5 was unrelated and not intended to be part of the pull request; sorry about that.
  • e4a0fc2 was intended to be for future-proofing. Odd that I didn't notice 8-day weeks in my testing; so it goes. As you note, it's probably overkill anyway.
  • e9317d5 was unrelated and not intended to be part of the pull request; sorry about that.
@eternicode

This comment has been minimized.

Show comment Hide comment
@eternicode

eternicode Apr 22, 2012

Contributor

@todofixthis alright, thanks for the clarifications. I'll fold the first two in (after my own testing), and leave the last two out. Thanks again!

Contributor

eternicode commented Apr 22, 2012

@todofixthis alright, thanks for the clarifications. I'll fold the first two in (after my own testing), and leave the last two out. Thanks again!

@eternicode eternicode closed this Apr 22, 2012

@intellix

This comment has been minimized.

Show comment Hide comment
@intellix

intellix Aug 14, 2013

Is there a reason Sunday appears twice?

Is there a reason Sunday appears twice?

jamesbrucepower pushed a commit to efundamentals/bootstrap-datepicker that referenced this pull request May 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment