-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(modal): add missing fields to NgbModalConfig
#3406
Conversation
This fixes compile error when a user tries to set any of this fields in a `NgbModalConfig` instance.
Codecov Report
@@ Coverage Diff @@
## master #3406 +/- ##
==========================================
+ Coverage 90.98% 91.05% +0.07%
==========================================
Files 95 95
Lines 2774 2774
Branches 515 515
==========================================
+ Hits 2524 2526 +2
+ Misses 190 189 -1
+ Partials 60 59 -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.
Thanks for the change!
Optionally, can we take an opportunity here and cover this change with contract check?
// src/modal/modal-config.spec.ts
import {inject} from '@angular/core/testing';
import {NgbModalConfig} from './modal-config';
describe('NgbModalConfig', () => {
it('should have sensible default values', inject([NgbModalConfig], (config: NgbModalConfig) => {
expect(config.ariaLabelledBy).toBeUndefined();
expect(config.backdrop).toBe(true);
expect(config.backdropClass).toBeUndefined();
expect(config.beforeDismiss).toBeUndefined();
expect(config.centered).toBeUndefined();
expect(config.container).toBeUndefined();
expect(config.injector).toBeUndefined();
expect(config.keyboard).toBe(true);
expect(config.scrollable).toBeUndefined();
expect(config.size).toBeUndefined();
expect(config.windowClass).toBeUndefined();
}));
});
LGTM!
Can you reschedule the the CI builds (when you follow the 'details' and then into CI, you should see 'restart build' button)? |
I've access to machine with IE, I'll try to look into this. |
It looks like false positive, I've configured local env for IE and tests passed (with some minor modification required in IE configuration to pass tooltips coverage, this will be added in separate PR):| yarn test --browsers IENoExtensions --configuration ie
yarn run v1.19.1
$ yarn check-format && yarn ngb:lint && yarn ngb:test --browsers IENoExtensions --configuration ie
$ ts-node --project misc/tsconfig.json misc/check-format.ts
$ ng lint ng-bootstrap
Linting "ng-bootstrap"...
All files pass linting.
$ ng test ng-bootstrap --code-coverage --source-map true --progress false --watch false --browsers IENoExtensions --configuration ie
12 10 2019 22:25:19.901:INFO [karma-server]: Karma v4.1.0 server started at http://0.0.0.0:9876/
12 10 2019 22:25:19.911:INFO [launcher]: Launching browsers IENoExtensions with concurrency unlimited
12 10 2019 22:25:19.977:INFO [launcher]: Starting browser IE
12 10 2019 22:25:33.722:INFO [IE 11.0.0 (Windows 10.0.0)]: Connected on socket drIoXh8tu3oBlH-0AAAA with id 83844884
IE 11.0.0 (Windows 10.0.0): Executed 1268 of 1268 SUCCESS (2 mins 9.038 secs / 2 mins 1.628 secs)
TOTAL: 1268 SUCCESS
TOTAL: 1268 SUCCESS
Done in 182.78s. |
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.
Hey, @gentoo90 !
Thanks for the PR, just one remark.
Build is fine, don't worry about it :)
Cheers,
Max
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 for contributing!
Will merge when Travis is green.
This fixes compile error when a user tries to set any of this fields in a
NgbModalConfig
instance.Before submitting a pull request, please make sure you have at least performed the following: