-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(timepicker): setting hour/minute/second step to undefined #2903
fix(timepicker): setting hour/minute/second step to undefined #2903
Conversation
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.
Hi, thanks for the PR!
Left a couple of comments.
src/timepicker/timepicker.ts
Outdated
@@ -96,6 +96,10 @@ export class NgbTimepicker implements ControlValueAccessor, | |||
disabled: boolean; | |||
model: NgbTime; | |||
|
|||
private _hourStep; |
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.
please add : number
type
src/timepicker/timepicker.ts
Outdated
@@ -136,7 +159,7 @@ export class NgbTimepicker implements ControlValueAccessor, | |||
*/ | |||
@Input() size: 'small' | 'medium' | 'large'; | |||
|
|||
constructor(config: NgbTimepickerConfig, private _ngbTimeAdapter: NgbTimeAdapter<any>) { | |||
constructor(private readonly config: NgbTimepickerConfig, private _ngbTimeAdapter: NgbTimeAdapter<any>) { |
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.
As config becomes private, please rename to _config
(see https://github.com/ng-bootstrap/ng-bootstrap/wiki/Contributions%3A-Code-conventions)
src/timepicker/timepicker.spec.ts
Outdated
}); | ||
})); | ||
|
||
it('should increment / decrement minutes by 7', async(() => { |
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.
Could you please simplify tests, as we should use async/await
or fakeAsync
(I know you were inspired by already exsiting ones, but now there is a shorter way to write) →
Before:
it('should increment / decrement minutes by 7', async(() => {
//...
fixture.detectChanges();
fixture.whenStable()
.then(() => {
fixture.detectChanges();
return fixture.whenStable();
})
.then(() => {
// ...
});
}));
After:
it('should increment / decrement minutes by 7', async(async () => {
// ...
fixture.detectChanges();
await fixture.whenStable();
fixture.detectChanges();
// ...
}));
src/timepicker/timepicker.ts
Outdated
@@ -114,17 +118,36 @@ export class NgbTimepicker implements ControlValueAccessor, | |||
/** | |||
* Number of hours to increase or decrease when using a button. | |||
*/ | |||
@Input() hourStep: number; | |||
@Input() set hourStep(step: number) { | |||
this._hourStep = isNumber(step) ? step : this.config.hourStep; |
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.
Maybe better isInteger()
?
Oh, also the build failed because of improper code formatting: https://travis-ci.org/ng-bootstrap/ng-bootstrap/builds/463821660#L508 You could run |
Thanks for the comments! I've updated the PR. |
Codecov Report
@@ Coverage Diff @@
## master #2903 +/- ##
==========================================
+ Coverage 91.16% 91.23% +0.06%
==========================================
Files 89 89
Lines 2863 2885 +22
Branches 466 475 +9
==========================================
+ Hits 2610 2632 +22
Misses 181 181
Partials 72 72
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.
LGTM, thanks!
Closes #2851
Also added some tests for step setting via component input properties