-
Notifications
You must be signed in to change notification settings - Fork 65
Reactive form demos for datepicker and timepicker #2204
Reactive form demos for datepicker and timepicker #2204
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2204 +/- ##
=======================================
Coverage 98.34% 98.34%
=======================================
Files 22 22
Lines 121 121
Branches 12 12
=======================================
Hits 119 119
Misses 2 2 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.
One small thing.
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 code looks great. I have some suggestions on improving the demo a tad. What do you think?
|
||
<h3> | ||
Reactive form timepicker | ||
</h3> |
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.
Should we make these headers consistent with the date demo? I noticed they're all <h2 class="sky-section-heading”>
, while all the headers on this page are using <h3>
.
this.selectedTime1 = undefined; | ||
this.reactiveTime.setValue(undefined); |
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 button that calls this method is labeled Clear selected times
, but it only clears the 1st and last selected times. Should it clear them all?
[formGroup]="reactiveForm" | ||
> | ||
<sky-timepicker | ||
#timePickerExample4 |
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.
Should these template tags be a little more descriptive instead of 1-2-3-4? Something like #reactivePicker
?
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.
🚢 🇮🇹
Issue: #2180
Docs: N/A