-
Notifications
You must be signed in to change notification settings - Fork 4.1k
feat(module:dropdown): support nzDestroyOnHidden #9580
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
Draft
WwwHhhYran
wants to merge
1
commit into
NG-ZORRO:master
Choose a base branch
from
WwwHhhYran:feat/support_nzDestroyOnHidden
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+64
−0
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the animation is disabled by the user, does that mean the
nzDestroyOnHidewill not take effect?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
理论上及时用户关闭了动画,但依然会触发 Angular 动画的
done事件,执行这个订阅流?不过写在这里确实不好,如果想重新 new 一个 portal 应该写成下面这样更好:但我现在发现一个问题,即使这里 new 了一个新的 portal,但实际上并没有销毁 dropdown menu 🤔,下一次打开依然是上一次的。这个 PR 我后续再改下
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nzDropdownMenu中展示的组件是通过<ng-content>投影过来的,其生命周期的管理权应该在它原始声明的位置,我们这里对 portal 和 overlayRef 的重建操作无法实现ng-content的重建,所以渲染的始终是ng-content中最开始的那个实例。不知道是否有其他方法来支撑☹️
nzDestroyOnHidden,我暂时还没有思路,如果没有方法的话,我可能会关闭这个 PRUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这一点我觉得是我们和 antd react 最大的差异。
antd dropdown 组件是通过传入 menu props,在 doprdown 组件内部渲染 menu,dropdown 能完整控制 menu 的生命周期;
而 zorro 是将 menu ref 传给 dropdown,即使 dropdown 能销毁 menu,但无法重新实例化 menu。
angular 本身我理解也不具备解决这类问题的方法,如果要支持,目前我觉得只有通过 menu props
WDYT cc @HyperLife1119
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
menu props 的方式其实并不适合 Angular,因为 antd 的 menu props 里可以直接传递 jsx,而 angular 只能传递 ng-template,用法会很繁琐
我理想中的 dropdown 组件应该是这样的:
优点:
removeChild(HostElement)来移除 DOM 节点(我发现过去 nz popover / tooltip 都是这么干的,很不干净)或者参考一下 angular aria menu:https://angular.dev/guide/aria/menu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
确实是一个好方法👍!不过如果这样实现的话,dropdown 现有的实现方式可能会有较大的变动。例如:
nz-dropdown-menu的 template 不再需要使用<ng-template>,直接渲染为:包括
nzPlacement,nzArrow等属性是否需要迁移到nz-dropdown-menu组件上,等等。这个改动会影响到现有用户的使用 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果 schematics 自动迁移实现成本过高,可以将旧的 dropdown 设置为 DropdownLegacyModule,像之前 NzInputNumberLegacyModule 那样,让用户自己在未来的几个版本内逐步手动迁移到新的 DropdownModule 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个大版本估计来不及搞了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我先转为 draft,后续考虑进行重构