-
Notifications
You must be signed in to change notification settings - Fork 65
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1732 +/- ##
==========================================
+ Coverage 99.98% 99.98% +<.01%
==========================================
Files 409 409
Lines 8495 8504 +9
Branches 1244 1245 +1
==========================================
+ Hits 8494 8503 +9
Misses 1 1
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.
Also, it might be a good idea to have a unit test "prove" that this input gets carried down to the calendar component.
@@ -26,6 +27,9 @@ export class SkyDatepickerComponent { | |||
@ViewChild(SkyDropdownComponent) | |||
public dropdown: SkyDropdownComponent; | |||
|
|||
@Input() | |||
public startingDay: number; |
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.
@blackbaud-conorwright Is there any way we can add this input to the datepicker-input.directive.ts
instead? The docs already mention that this input exists on the directive...
https://developer.blackbaud.com/skyux/components/datepicker#datepicker-input-directive-properties
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.
moved. Though it feels weird to me that the calendar's starting day would be on the Input at all, I can see why it would be weird to have an input somewhere the rest weren't
@blackbaud-conorwright Looks like there's a TSLint error: Error at /home/travis/build/blackbaud/skyux2/src/modules/datepicker/datepicker.component.ts:5:3: 'Input' is declared but never used." |
ah yeah, sorry. I missed cleaning up the import 😛 |
@@ -50,6 +51,10 @@ export class SkyDatepickerComponent { | |||
this.maxDate = _maxDate; | |||
} | |||
|
|||
public setStartingDay(_startingDay: number) { |
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.
Curious if there's a reason we're not using get
and set
for this property? Instead of a setStartingDay
...
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.
huh yeah, they're also just public so we currently don't need the methods for this and the max/min dates. I can swap them all to getters and setters
Fixes #1731