Skip to content
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

Merged
merged 3 commits into from Oct 18, 2019

Conversation

gentoo90
Copy link
Contributor

@gentoo90 gentoo90 commented Oct 8, 2019

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:

  • read and followed the CONTRIBUTING.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

This fixes compile error when a user tries to set any of this fields in a
`NgbModalConfig` instance.
@codecov-io
Copy link

codecov-io commented Oct 8, 2019

Codecov Report

Merging #3406 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#e2e 55.55% <100%> (ø) ⬆️
#unit 88.13% <100%> (+0.07%) ⬆️
Impacted Files Coverage Δ
src/modal/modal-config.ts 100% <100%> (ø) ⬆️
src/toast/toast.ts 95.65% <0%> (ø) ⬆️
src/buttons/checkbox.ts 100% <0%> (ø) ⬆️
src/rating/rating.ts 100% <0%> (+3.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36918b5...d0c1baa. Read the comment docs.

Copy link
Contributor

@peterblazejewicz peterblazejewicz left a 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!

@gentoo90
Copy link
Contributor Author

gentoo90 commented Oct 9, 2019

Done.

Datapicker test failed, but I didn't touch that part of code

изображение

@peterblazejewicz
Copy link
Contributor

Can you reschedule the the CI builds (when you follow the 'details' and then into CI, you should see 'restart build' button)?
I've just got same false positive with modals on the updated Catalina box, went away on the on rerun.

@gentoo90
Copy link
Contributor Author

gentoo90 commented Oct 9, 2019

No Restart button. Guess it's only for the owner, and others just don't have permissions

изображение

@peterblazejewicz
Copy link
Contributor

I've access to machine with IE, I'll try to look into this.

@peterblazejewicz
Copy link
Contributor

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.

Copy link
Member

@maxokorokov maxokorokov left a 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

src/modal/modal-config.ts Outdated Show resolved Hide resolved
Copy link
Member

@maxokorokov maxokorokov left a 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.

@maxokorokov maxokorokov merged commit 7324083 into ng-bootstrap:master Oct 18, 2019
@gentoo90 gentoo90 deleted the modal-config-fix branch October 18, 2019 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants