-
Notifications
You must be signed in to change notification settings - Fork 11
Fixed calendar weekdays not being static #41
Conversation
@@ -51,7 +51,7 @@ export class SkyDatepickerComponent implements OnDestroy { | |||
public dateChange = new EventEmitter<Date>(); | |||
public maxDate: Date; | |||
public minDate: Date; | |||
public startingDay: number; | |||
public startingDay: number = 0; |
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.
What do you think about setting the default in the calendar component, so that if undefined
is passed (or set) from on-high, it will always resolve to zero
when it reaches the calendar.
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.
The calendar component has its own spec, too.
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.
Good call. Done.
I also noticed that with what I had that if the user actually bound to undefined we would still use that. My new version uses a getter/setter in order to ensure we use "0" if undefined is passed in.
You also may notice that I no longer have a test around this. This is because we really already had a test for this (line 131 of the datepicker-calendar spec); however, it was passing before because our test component was setting 0 by default. I have changed the test component to not send in a value unless specifically set in a test (which we do have 2 tests doing this).
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 26 26
Lines 1311 1314 +3
Branches 199 200 +1
=====================================
+ Hits 1311 1314 +3
Continue to review full report at Codecov.
|
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.
Is there already a unit test to confirm that the starting day defaults to zero?
Yup. See my comment on your last comment. There was already one but the test component was manually setting the |
No description provided.