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: tooltip will change the position with the change of origin in custom element #4613

Closed
wants to merge 3 commits into from
Closed

feat: tooltip will change the position with the change of origin in custom element #4613

wants to merge 3 commits into from

Conversation

Eve-Sama
Copy link
Contributor

@Eve-Sama Eve-Sama commented Dec 30, 2019 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

Issue Number: #3842

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@zorro-bot
Copy link

zorro-bot bot commented Dec 30, 2019

This preview will be available after the TravisCI is passed.

@codecov
Copy link

codecov bot commented Dec 30, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@44da730). Click here to learn what that means.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4613   +/-   ##
=========================================
  Coverage          ?   93.23%           
=========================================
  Files             ?      520           
  Lines             ?    12828           
  Branches          ?     2014           
=========================================
  Hits              ?    11960           
  Misses            ?      452           
  Partials          ?      416
Impacted Files Coverage Δ
components/tooltip/nz-tooltip.module.ts 100% <ø> (ø)
components/popover/nz-popover.directive.ts 100% <100%> (ø)
components/tooltip/nz-tooltip-scroll.directive.ts 100% <100%> (ø)
components/popconfirm/nz-popconfirm.directive.ts 100% <100%> (ø)
components/tooltip/nz-tooltip.directive.ts 100% <100%> (ø)
components/tooltip/nz-tooltip-base.directive.ts 94.89% <66.66%> (ø)

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 44da730...8dabd1a. Read the comment docs.

wzhudev
wzhudev reviewed Dec 30, 2019
View reviewed changes
components/tooltip/nz-tooltip-base.directive.ts
@@ -116,6 +125,7 @@ export abstract class NzTooltipBaseDirective implements OnChanges, OnInit, OnDes
protected hostView: ViewContainerRef,
protected resolver: ComponentFactoryResolver,
protected renderer: Renderer2,
protected nzTooltipScrollDirective: NzTooltipScrollDirective,
Copy link
Member

Choose a reason for hiding this comment

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

This should be optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have add @optional in NzTooltipDirective

components/tooltip/nz-tooltip.spec.ts Outdated Show resolved Hide resolved
docs/faq.zh-CN.md Outdated Show resolved Hide resolved
components/tooltip/nz-tooltip-base.directive.ts Outdated Show resolved Hide resolved
wzhudev
wzhudev requested changes Dec 30, 2019
View reviewed changes
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.

另外这个不是个 fix，而是个 feat，因为你引入了新的 API

@Eve-Sama Eve-Sama changed the title bugfix: tooltip will change the position with the change of origin in custom element feat: tooltip will change the position with the change of origin in custom element Jan 1, 2020
@Eve-Sama
Copy link
Contributor Author

Eve-Sama commented Jan 1, 2020

I have written a test named should support scoller expect for body and it passed, the screen shows 0 failures. But the console of Karma shows that Unhandled Promise rejection: The code should be running in the fakeAsync zone to call this function ; Zone: ProxyZone ; Task: Promise.then ; Value: Error: The code should be running in the fakeAsync zone to call this function.

I don't know why, who can help me?

@Eve-Sama Eve-Sama requested a review from wzhudev January 1, 2020 07:43
@weekly-digest weekly-digest bot mentioned this pull request Jan 26, 2020
Closed
@wzhudev
Copy link
Member

wzhudev commented Feb 29, 2020

Hi there! Could you please rebase to the lattest commit on the master branch? We've refactored this comopnent.

@wzhudev
Copy link
Member

wzhudev commented Feb 29, 2020

Emmmm 似乎使用 CDK 提供的 CdkScrollable 就可以了
https://github.com/angular/components/blob/d83059b6deb021193c0964eecf75452ffaf1dd0a/src/cdk/scrolling/scroll-dispatcher.ts#L54

@Eve-Sama
Copy link
Contributor Author

I'll rebase it when I am free. And I will find some information about CdkScrollable to make up a better resolution.

@weekly-digest weekly-digest bot mentioned this pull request Mar 1, 2020
Closed
@Eve-Sama
Copy link
Contributor Author

先关闭这个PR了, 手头已安排的事情较多, 最近都没时间. 之后有空了我会重新基于现有版本提一个PR.

@Eve-Sama Eve-Sama closed this Apr 19, 2020
@Eve-Sama Eve-Sama deleted the bugfix/tooltip-scroll branch April 23, 2020 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wzhudev wzhudev Awaiting requested review from wzhudev

Assignees

@wzhudev wzhudev

Labels
PR: unreviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Eve-Sama @wzhudev