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

chore(module:message,notification): refactor #4723

Merged
merged 3 commits into from
Mar 15, 2020

Conversation

wzhudev
Copy link
Member

@wzhudev wzhudev commented Jan 25, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] 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: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

@zorro-bot
Copy link

zorro-bot bot commented Jan 25, 2020

This preview will be available after the AzureCI is passed.

@codecov
Copy link

codecov bot commented Jan 25, 2020

Codecov Report

Merging #4723 into master will increase coverage by 0.02%.
The diff coverage is 90.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4723      +/-   ##
==========================================
+ Coverage   92.93%   92.96%   +0.02%     
==========================================
  Files         505      511       +6     
  Lines       12718    13027     +309     
  Branches     1963     1971       +8     
==========================================
+ Hits        11819    12110     +291     
- Misses        474      484      +10     
- Partials      425      433       +8     
Impacted Files Coverage Δ
components/core/config/config.ts 100.00% <ø> (ø)
components/message/message.module.ts 100.00% <ø> (ø)
components/message/message.service.module.ts 100.00% <ø> (ø)
components/message/message.service.ts 100.00% <ø> (ø)
components/notification/notification.module.ts 100.00% <ø> (ø)
...onents/notification/notification.service.module.ts 100.00% <ø> (ø)
components/notification/notification.service.ts 100.00% <ø> (ø)
components/message/message-container.component.ts 88.63% <75.00%> (ø)
components/message/message.component.ts 88.88% <88.88%> (ø)
components/message/base.service.ts 100.00% <100.00%> (ø)
... and 76 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 72d320b...a2c6b80. Read the comment docs.

Copy link
Member

@hsuanxyz hsuanxyz left a comment

Choose a reason for hiding this comment

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

Preview 打不开组件,不知道是不是样式没同步的原因。

private eraseTimingStart: number;
private eraseTTL: number; // Time to live.

constructor(private _messageContainer: NzMessageContainerComponent, protected cdr: ChangeDetectorRef) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
constructor(private _messageContainer: NzMessageContainerComponent, protected cdr: ChangeDetectorRef) {}
constructor(private messageContainer: NzMessageContainerComponent, protected cdr: ChangeDetectorRef) {}

this.nzConfigService
.getConfigChangeEventForComponent(NZ_CONFIG_COMPONENT_NAME)
.subscribe(() => this.updateConfig());
this.nzConfigService.getConfigChangeEventForComponent(NZ_CONFIG_COMPONENT_NAME).subscribe(() => this.updateConfig());
Copy link
Member

Choose a reason for hiding this comment

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

这里不需要取消订阅吗?

@@ -32,7 +32,20 @@ const NZ_NOTIFICATION_DEFAULT_CONFIG: Required<NotificationConfig> = {
selector: 'nz-notification-container',
exportAs: 'nzNotificationContainer',
preserveWhitespaces: false,
templateUrl: './nz-notification-container.component.html'
template: `
<div class="ant-notification ant-notification-topLeft" [style.top]="top" [style.left]="'0px'">
Copy link
Member

Choose a reason for hiding this comment

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

这里是否需要判断对应方向的 length,不然外层容器始终存在。

image

Wendell Hu added 2 commits March 11, 2020 00:26
@vthinkxie vthinkxie merged commit 0f85549 into NG-ZORRO:master Mar 15, 2020
@wzhudev wzhudev deleted the msg-noti-9.0 branch March 15, 2020 09:38
Ricbet pushed a commit to Ricbet/ng-zorro-antd that referenced this pull request Apr 9, 2020
* feat(module:notification): support opening at different places

close NG-ZORRO#4338

chore(module:message,notification): refactor

* chore: test with cdr

* chore: remove console
hsuanxyz pushed a commit to hsuanxyz/ng-zorro-antd that referenced this pull request Aug 5, 2020
* feat(module:notification): support opening at different places

close NG-ZORRO#4338

chore(module:message,notification): refactor

* chore: test with cdr

* chore: remove console
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.

3 participants