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.

Sign up for GitHub

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

Jump to bottom

feat(module:root): make nz-root optional #36

Merged
merged 1 commit into from
Aug 24, 2017
Merged

feat(module:root): make nz-root optional #36

merged 1 commit into from
Aug 24, 2017

Conversation

trotyl
Copy link
Contributor

@trotyl trotyl commented Aug 16, 2017 •
edited
Loading

Closes #34 .

API in detail:

For NgZorroAntdModule.forRoot() method, there is an optional parameter in signature { extraFontName: string, extraFontUrl: string }, like:

const options = { extraFontName: 'some-name', extraFontUrl: 'some-url' };

imports: [
  NgZorroAntdModule.forRoot(options)
]

It would be same as using <nz-root nzExtraFontName="some-name" nzExtraFontUrl="some-url">.

Meanwhile, user can manually provide an NZ_ROOT_CONFIG if they don't want to import the entire lib.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 52.163% when pulling d3b4f93 on trotyl:optional-root into 32234e4 on NG-ZORRO:master.

@trotyl trotyl closed this Aug 16, 2017
@trotyl trotyl reopened this Aug 16, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 52.447% when pulling 87c53fc on trotyl:optional-root into 32234e4 on NG-ZORRO:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 52.447% when pulling 87c53fc on trotyl:optional-root into 32234e4 on NG-ZORRO:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 52.428% when pulling 86208d0 on trotyl:optional-root into 32234e4 on NG-ZORRO:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 52.357% when pulling 91a2478 on trotyl:optional-root into 32234e4 on NG-ZORRO:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.4%) to 39.812% when pulling d924a45 on trotyl:optional-root into a1b4e84 on NG-ZORRO:master.

@trotyl
Copy link
Contributor Author

trotyl commented Aug 17, 2017

The coverage has increased, but since the original coverage is fake (most of the source files didn't being count into), it has wrong result in the report.

@trotyl trotyl changed the title [WIP] Make <nz-root> component optional feat(root): make nz-root optional Aug 17, 2017
@vthinkxie
Copy link
Member

original coverage count is not correct.

@trotyl trotyl changed the title feat(root): make nz-root optional [WIP] feat(root): make nz-root optional Aug 17, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-12.2%) to 39.933% when pulling a4c467c on trotyl:optional-root into 5f861bd on NG-ZORRO:master.

@trotyl trotyl changed the title [WIP] feat(root): make nz-root optional feat(root): make nz-root optional Aug 17, 2017
@trotyl trotyl changed the title feat(root): make nz-root optional feat(module:root): make nz-root optional Aug 17, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-12.3%) to 39.874% when pulling bb7078d on trotyl:optional-root into 44865c2 on NG-ZORRO:master.

vthinkxie
vthinkxie reviewed Aug 22, 2017
View reviewed changes
src/components/root/nz-root.module.ts

export class NzRootModule {
constructor(@Inject(DOCUMENT) _document: Document, injector: Injector, resolver: ComponentFactoryResolver) {
const componentFactory = resolver.resolveComponentFactory(NzRootStyleComponent);
Copy link
Member

@vthinkxie vthinkxie Aug 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

users may insert nz-root manually , maybe a judgement condition should be added here.

Copy link
Contributor Author

@trotyl trotyl Aug 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already been resolved on NzRootComponent side:

this.nzExtraFontName && this.nzExtraFontUrl && !this.options.

@vthinkxie
Copy link
Member

vthinkxie commented Aug 22, 2017 •
edited
Loading

docs needed after nz-root is optional.

@trotyl
Copy link
Contributor Author

trotyl commented Aug 22, 2017

Should it be:

  • Make <nz-root> the default option, with this being an alternative. Or
  • Make this the default option, with <nz-root> being an alternative. Or
  • Make this the suggested way, with <nz-root> being a deprecated option. Or
  • Make this the only documented option, with <nz-root> not mentioned anywhere?

And should we change the way that showcase app uses?

@vthinkxie
Copy link
Member

what about removing nz-root from the document and tell users that it has been deprecated on the changelog page, and the showcase app should also change.

@trotyl
Copy link
Contributor Author

trotyl commented Aug 22, 2017

Sounds good, would update it soon.

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.2%) to 39.993% when pulling a84becc on trotyl:optional-root into 143c080 on NG-ZORRO:master.

@vthinkxie vthinkxie merged commit 9de3de1 into NG-ZORRO:master Aug 24, 2017
suraciii pushed a commit to suraciii/ng-zorro-antd that referenced this pull request Aug 30, 2017
Merge branch 'master' into dev
d217b08 
* master:
  refactor(module:dropdown): improve performance (NG-ZORRO#148)
  feat(module:tooltip,popconfirm,popover): support OnPush (NG-ZORRO#143)
  release(0.5.0-rc.3): pre-release 0.5.0-rc.3 (NG-ZORRO#166)
  fix(module:carousel): fix carousel auto play bug (NG-ZORRO#164)
  fix(module:input): fix input disabled style bug (NG-ZORRO#160)
  fix(module:input): fix input disabled style bug (NG-ZORRO#108)
  fix(module:affix,anchor,back-top): fix and improve rxjs usage (NG-ZORRO#159)
  feat(module:affix&anchor&back-top&avatar): add components to library (NG-ZORRO#88)
  fix(module:select): fix select reset bug in form (NG-ZORRO#153)
  fix(module:pagination) fix pagination double binding (NG-ZORRO#146)
  fix(module:select): fix option incorrect active status (NG-ZORRO#141)
  feat(module:root): make nz-root optional (NG-ZORRO#36)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vthinkxie vthinkxie vthinkxie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@trotyl @coveralls @vthinkxie