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(module:message): fix lazy load problem #3797

Merged
merged 3 commits into from
Jul 16, 2019
Merged

Conversation

hsuanxyz
Copy link
Member

@hsuanxyz hsuanxyz commented Jul 15, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #3794

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@netlify
Copy link

netlify bot commented Jul 15, 2019

Deploy preview for ng-zorro-master ready!

Built with commit 22e4073

https://deploy-preview-3797--ng-zorro-master.netlify.com

@netlify
Copy link

netlify bot commented Jul 15, 2019

Deploy preview for ng-zorro-master ready!

Built with commit bb69d20

https://deploy-preview-3797--ng-zorro-master.netlify.com

@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #3797 into master will decrease coverage by <.01%.
The diff coverage is 98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3797      +/-   ##
==========================================
- Coverage   95.37%   95.36%   -0.01%     
==========================================
  Files         706      709       +3     
  Lines       14515    14574      +59     
  Branches     1915     1924       +9     
==========================================
+ Hits        13843    13898      +55     
- Misses        244      246       +2     
- Partials      428      430       +2
Impacted Files Coverage Δ
components/message/nz-message.service.ts 100% <100%> (+2.04%) ⬆️
components/message/public-api.ts 100% <100%> (ø) ⬆️
components/message/nz-message.service.module.ts 100% <100%> (ø)
components/message/nz-message.module.ts 100% <100%> (ø) ⬆️
components/breadcrumb/nz-breadcrumb.component.ts 92% <100%> (ø) ⬆️
components/notification/nz-notification.service.ts 100% <100%> (ø) ⬆️
...nts/notification/nz-notification.service.module.ts 100% <100%> (ø)
components/notification/public-api.ts 100% <100%> (ø) ⬆️
components/notification/nz-notification.module.ts 100% <100%> (ø) ⬆️
components/message/nz-message-base.service.ts 97.14% <97.14%> (ø)
... and 20 more

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 feae069...bb69d20. Read the comment docs.

@hsuanxyz hsuanxyz requested a review from wzhudev July 16, 2019 07:52
Copy link
Member

@wzhudev wzhudev left a comment

Choose a reason for hiding this comment

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

动态修改配置的问题似乎和当前的 bug 冲突

this._container.setConfig(config);
}

protected _generateMessageId(): string {
Copy link
Member

Choose a reason for hiding this comment

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

这个变成类的静态 public 方法会不会更合适?

return resultMessage;
}

config(config: MessageConfig): void {
Copy link
Member

@wzhudev wzhudev Jul 16, 2019

Choose a reason for hiding this comment

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

如果用户在两个 FeatureModule 里都引入了 NzMessageModule,那么在其中一个 module 调用此方法时,对其他 module 里的 message container 应该不会生效,因为每个 module 都会创建自己的 service 实例。

Copy link
Member

Choose a reason for hiding this comment

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

等 global config 再来修好了

Copy link
Member

@wzhudev wzhudev left a comment

Choose a reason for hiding this comment

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

LGTM

@wzhudev wzhudev merged commit 679fdea into NG-ZORRO:master Jul 16, 2019
Ricbet pushed a commit to Ricbet/ng-zorro-antd that referenced this pull request Apr 9, 2020
* fix(module:message): fix lazy load problem

close NG-ZORRO#3794

* test(module:message): fix test

* fix(module:message): fix module
hsuanxyz added a commit to hsuanxyz/ng-zorro-antd that referenced this pull request Aug 5, 2020
* fix(module:message): fix lazy load problem

close NG-ZORRO#3794

* test(module:message): fix test

* fix(module:message): fix module
@hsuanxyz hsuanxyz deleted the fix/entry branch August 5, 2020 02:28
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.

2 participants