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

refactor(module:dropdown): improve performance #148

Merged
merged 1 commit into from
Aug 29, 2017

Conversation

Brooooooklyn
Copy link
Contributor

@Brooooooklyn Brooooooklyn commented Aug 25, 2017

  1. use Observable to replace Subject, which could dispose eventListener.
  2. use ChangeDetectionStrategy.OnPush to reduce check times.

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)
[x] 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
[x] No

Other information

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 40.073% when pulling 88dee68 on Brooooooklyn:refactor/dropdown into b442863 on NG-ZORRO:master.

import { debounceTime } from 'rxjs/operator/debounceTime';
import 'rxjs/add/operator/debounceTime';
import 'rxjs/add/operator/do';
import 'rxjs/add/operator/merge';
Copy link
Contributor

Choose a reason for hiding this comment

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

Library should not use rxjs patching mechanism as suggested in angular/components#5314

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 40.073% when pulling 88dee68 on Brooooooklyn:refactor/dropdown into b442863 on NG-ZORRO:master.

} from '@angular/core';
import { Subject } from 'rxjs/Subject';
import { debounceTime } from 'rxjs/operator/debounceTime';
import 'rxjs/add/operator/debounceTime';
Copy link
Contributor

@trotyl trotyl Aug 25, 2017

Choose a reason for hiding this comment

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

Library should not use rxjs patching mechanism as suggested in angular/components#2622, angular/components#2652, angular/components#3716, angular/components#5314 (original line has been modified)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 40.066% when pulling 35d92a2 on Brooooooklyn:refactor/dropdown into b442863 on NG-ZORRO:master.

@LinboLen
Copy link
Contributor

are you sure it works?
improve performance ?

this._renderer.listen(this._nzOrigin.elementRef.nativeElement, 'mouseenter', () => this._show());
this._renderer.listen(this._nzOrigin.elementRef.nativeElement, 'mouseleave', () => this._hide());
mouse$ = Observable.create((observer: Observer<boolean>) => {
const disposeMouseEnter = this._renderer.listen(this._nzOrigin.elementRef.nativeElement, 'mouseenter', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should check whether will fire ngzone. although this component will not check

Copy link
Contributor

Choose a reason for hiding this comment

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

Zone propagation for RxJS has been fixed on Zone.js side already.

1. use Observable to replace Subject, which could dispose eventListener.
2. use ChangeDetectionStrategy.OnPush to reduce check times.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 40.346% when pulling f88c1fa on Brooooooklyn:refactor/dropdown into 8bbc82a on NG-ZORRO:master.

@trotyl
Copy link
Contributor

trotyl commented Aug 29, 2017

Concerns irrelevant to this PR, currently all event triggers are generalized and applied to a debounce of 300ms, but theoretically it should not apply to click, which should be reacted instantly.

@trotyl trotyl merged commit 29a5cbf into NG-ZORRO:master Aug 29, 2017
suraciii pushed a commit to suraciii/ng-zorro-antd that referenced this pull request Aug 30, 2017
* 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)
@vthinkxie vthinkxie mentioned this pull request Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants