motss / app-datepicker

Datepicker element built with Google's lit and Material Design 2021
https://npm.im/app-datepicker
MIT License
180 stars 51 forks source link

PR for bug 90 (2) #92

Closed userquin closed 4 years ago

userquin commented 7 years ago

Fixes #90

see commit comments for changes


This change is Reviewable

userquin commented 7 years ago

I have to modify a few things: basically I must update _isDisableDays parameters calls to use minDateObj and maxDateObj instead of minDate and maxDate respectively. You can modify them yourself or wait until I make the push with these changes.

userquin commented 7 years ago

I have it working and with no breaking changes, that is, min, max and disable dates can be any dateString Date constructor accepts (like your original): app-datepicker will convert properly to client TZ and do the logic. The demo page will remain with ISO date format on min, max and disable dates I also include a remark in the documentation.

I'm finishing some tests with different timezones... As soon as possible I'll make the push with the changes.

userquin commented 7 years ago

I think you should add to your travis configuration file 2 new tests, adding under env directive 3 TZ entries, for example (I have never tested this):

env:
  - TZ=Asia/Singapore
  - TZ=Europe/Madrid
  - TZ=America/Los_Angeles

You can read more about this:

https://docs.travis-ci.com/user/environment-variables/#Defining-Multiple-Variables-per-Item

motss commented 7 years ago

@userquin Isn't this one can be set directly on the .travis.yml file?

userquin commented 7 years ago

I think you can, but not tested... Take a look at: https://docs.travis-ci.com/user/languages/javascript-with-nodejs/#Ember-Apps For Ember configures env and matrix env...

userquin commented 7 years ago

Sorry, I did not understand you, yes add it at the end of the .travis.yml

userquin commented 7 years ago

If it works, we can add some failing tests for known dates, for example for Madrid using october and september 1995 and for the rest we need to find suitable failing scenarios, so the tests will check all works as expected...

motss commented 7 years ago

@userquin From the example you provided, those can be set in the .travis.yml https://github.com/motss/app-datepicker/blob/master/.travis.yml.

userquin commented 7 years ago

Are you reviewing the push? I have found an errata in _isDisableDays function:

1128: if (!_isDisableDates && this._isNumber(_item)) {

muts be

1128: if (!_isDisableDays && this._isNumber(_item)) {

and another in _computeDaysOfWeek function:

1168: var _firstDayOfWeek = _firstDayOfWeek > 0 && _firstDayOfWeek < 7 ? _firstDayOfWeek : 0;
1187: var _sliced = _dow.slice(_firstDayOfWeek);
1188: var _rest = _dow.slice(0, _firstDayOfWeek);

muts be (or reassign the _firstDayOfWeek to the expression)

1168: var __firstDayOfWeek = _firstDayOfWeek > 0 && _firstDayOfWeek < 7 ? _firstDayOfWeek : 0;
1187: var _sliced = _dow.slice(__firstDayOfWeek);
1188: var _rest = _dow.slice(0, __firstDayOfWeek);
motss commented 7 years ago

what's wrong here.

userquin commented 7 years ago

the first one is not wrong is to avoid evaluate max, min or find in the list if already disabled by week day

last code has two vars with the same name, first method parameter and 1168: var _firstDayOfWeek = _firstDayOfWeek > 0 && _firstDayOfWeek < 7 ? _firstDayOfWeek : 0;

motss commented 4 years ago

Closing this in favor of v4. Highly recommend using v4 even if you are using polymer.

Thank you for the support.