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

feat: Support menuTab #339

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: Support menuTab #339

wants to merge 2 commits into from

Conversation

zombieJ
Copy link
Member

@zombieJ zombieJ commented Nov 25, 2020

ref ant-design/ant-design#27721

Add menuTab

@vercel
Copy link

vercel bot commented Nov 25, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/react-component/tabs/5nrchd8uy
✅ Preview: https://tabs-git-title.react-component.vercel.app

@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

Merging #339 (6cec9f8) into master (cda1a7f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #339   +/-   ##
=======================================
  Coverage   98.72%   98.72%           
=======================================
  Files          13       13           
  Lines         548      549    +1     
  Branches      137      138    +1     
=======================================
+ Hits          541      542    +1     
  Misses          7        7           
Impacted Files Coverage Δ
src/TabPanelList/TabPane.tsx 100.00% <ø> (ø)
src/TabNavList/OperationNode.tsx 100.00% <100.00%> (ø)

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 cda1a7f...6cec9f8. Read the comment docs.

@kerm1it
Copy link
Member

kerm1it commented Nov 25, 2020

对 tabs 不太了解,没看懂这个功能😂

@kerm1it
Copy link
Member

kerm1it commented Nov 25, 2020

是不是设置超出后下拉时 tab 项的显示?

@kerm1it
Copy link
Member

kerm1it commented Nov 26, 2020

那我觉得 menuTab 这个属性不太好理解。

是不是可以 允许 tab 属性为一个函数,返回一个 ReactNode,参数可以为 是否溢出的状态,这样以来,用户可以使用一个 ReactNode 完成渲染,其中的拖拽事件、tooltip 等副作用可以根据是否溢出的状态作选择。

@hengkx
Copy link
Member

hengkx commented Nov 26, 2020

怪怪的

@zombieJ
Copy link
Member Author

zombieJ commented Nov 26, 2020

tab 这个名字我觉得也不太好,感觉最初应该设计叫 title,这样另一个就可以叫 dropdownTitle

@zombieJ
Copy link
Member Author

zombieJ commented Nov 26, 2020

那我觉得 menuTab 这个属性不太好理解。

是不是可以 允许 tab 属性为一个函数,返回一个 ReactNode,参数可以为 是否溢出的状态,这样以来,用户可以使用一个 ReactNode 完成渲染,其中的拖拽事件、tooltip 等副作用可以根据是否溢出的状态作选择。

这样对用户而言就太复杂了一些,目前看这个需求其实就是比较简单的,一个标准模式下,一个下拉菜单里。

@rinick
Copy link
Contributor

rinick commented Nov 26, 2020

是不是可以 允许 tab 属性为一个函数,返回一个 ReactNode,参数可以为 是否溢出的状态,这样以来,用户可以使用一个 ReactNode 完成渲染,其中的拖拽事件、tooltip 等副作用可以根据是否溢出的状态作选择。

这样对用户而言就太复杂了一些,目前看这个需求其实就是比较简单的,一个标准模式下,一个下拉菜单里。

我觉得会遇到这个问题的用户本身在做的事情应该就很复杂了,应该不会介意。

不管是menuTab,或者dropdownTab,或者返回函数,都很难理解,要解决这个问题大概只能在antdesign的Tab示例里加个例子

@zombieJ
Copy link
Member Author

zombieJ commented Nov 26, 2020

例子是会加的,我是觉得直接的属性名字会更简单。如果没有 tab 属性,我会倾向于:

title= ReactNode | { title: ReactNode, dropdownTitle: ReactNode };

@hengkx
Copy link
Member

hengkx commented Nov 26, 2020

有效果图吗 来一张

@md5pk
Copy link

md5pk commented Nov 26, 2020

例子是会加的,我是觉得直接的属性名字会更简单。如果没有 tab 属性,我会倾向于:

title= ReactNode | { title: ReactNode, dropdownTitle: ReactNode };

这。。。破坏向下兼容?

还是在dropdownTab和回调函数里选一个吧。

或者可不可以像renderTabBar那样,加一个renderTabMenu的回调函数,让用户自己实现特殊的溢出菜单?

@kerm1it
Copy link
Member

kerm1it commented Nov 26, 2020

那我觉得 menuTab 这个属性不太好理解。
是不是可以 允许 tab 属性为一个函数,返回一个 ReactNode,参数可以为 是否溢出的状态,这样以来,用户可以使用一个 ReactNode 完成渲染,其中的拖拽事件、tooltip 等副作用可以根据是否溢出的状态作选择。

这样对用户而言就太复杂了一些,目前看这个需求其实就是比较简单的,一个标准模式下,一个下拉菜单里。

需求本身确实很简单,我是看了那个 issue,用户使用的时候,主要是希望事件,tooltip这些副作用,不要在下拉框里面出现,dom 本身其实没有多少区别,menuTab 就需要用户在 tab 的基础上复制一份 dom 然后再去控制事件、tooltip等副作用,感觉这样对用户不太友好。

menuTab 或者函数这样的本来就很难理解,用户用的时候肯定需要查文档的。

@rinick
Copy link
Contributor

rinick commented Nov 26, 2020

那我觉得 menuTab 这个属性不太好理解。
是不是可以 允许 tab 属性为一个函数,返回一个 ReactNode,参数可以为 是否溢出的状态,这样以来,用户可以使用一个 ReactNode 完成渲染,其中的拖拽事件、tooltip 等副作用可以根据是否溢出的状态作选择。

这样对用户而言就太复杂了一些,目前看这个需求其实就是比较简单的,一个标准模式下,一个下拉菜单里。

需求本身确实很简单,我是看了那个 issue,用户使用的时候,主要是希望事件,tooltip这些副作用,不要在下拉框里面出现,dom 本身其实没有多少区别,menuTab 就需要用户在 tab 的基础上复制一份 dom 然后再去控制事件、tooltip等副作用,感觉这样对用户不太友好。

鼠标事件还比较容易禁,如果tab用了一些css和icon想不在menu里显示就很难。
我最大的问题是在tab里用到getRef,因为两处的ref冲突,直接runtime error

因为我要做的事情太复杂,实在是无法绕开ref
https://github.com/ticlo/rc-dock

@zombieJ
Copy link
Member Author

zombieJ commented Nov 27, 2020

例子是会加的,我是觉得直接的属性名字会更简单。如果没有 tab 属性,我会倾向于:

title= ReactNode | { title: ReactNode, dropdownTitle: ReactNode };

这。。。破坏向下兼容?

还是在dropdownTab和回调函数里选一个吧。

或者可不可以像renderTabBar那样,加一个renderTabMenu的回调函数,让用户自己实现特殊的溢出菜单?

我说的是 “如果没有 tab 属性,我会倾向于”,不是说要改成这样……

@zombieJ
Copy link
Member Author

zombieJ commented Nov 27, 2020

复制 dom 指的是什么?这边其实就是设置个下拉菜单里的 title,应该不涉及复制的。

@kerm1it
Copy link
Member

kerm1it commented Nov 27, 2020

我不知道我理解的对不对

比如说现在有个 TabPane,tab 属性为一个带 tooltip 的 dom 节点这时候如果用户想在下拉菜单的时候不显示 tooltip,通常的做法可能会是把 tooltip 的 子元素提取出来,给 tab 传的时候外面包裹上 tooltip,给 menuTab 不包裹 tolltip。

第二种情况是,用户给 tab 的 dom 节点加了一些事件,又想在传给 menuTab 的时候取消这些事件,是不是就需要传给 tab 和 menuTab 相同的 dom,然后一个有事件,一个没有事件?

@zombieJ
Copy link
Member Author

zombieJ commented Nov 27, 2020

这个不用担心,Tooltip 是用的 cloneElement。另外,即便不是 Tooltip,React 内部从 ReactElement 到 dom node 也是有一个转译过程。render 里写的 Element 并不是最终的 fiber 节点。

只有上面提到的给节点加 ref 会遇到两个节点相互覆盖的问题,但是现在的方案也没有关它。

@vincentLiuxiang
Copy link

vincentLiuxiang commented Jun 12, 2021

我不知道我理解的对不对

比如说现在有个 TabPane,tab 属性为一个带 tooltip 的 dom 节点这时候如果用户想在下拉菜单的时候不显示 tooltip,通常的做法可能会是把 tooltip 的 子元素提取出来,给 tab 传的时候外面包裹上 tooltip,给 menuTab 不包裹 tolltip。

第二种情况是,用户给 tab 的 dom 节点加了一些事件,又想在传给 menuTab 的时候取消这些事件,是不是就需要传给 tab 和 menuTab 相同的 dom,然后一个有事件,一个没有事件?

hi 各位, 我们的产品也遇到了这个问题,就是不希望 drop down 里的 tab 有 tooltips,或者 drop down 里的 tab 的 tooltips 显示的位置可以自由控制,这就需要区别 render 正常 tab 和 在 drop down 里的 tab 了。这个 pr 的方案,或者把 tab 属性定义为这个方法,并给一个参数来区分当前 tab 是正常 tab 还是折叠起来就满足需求了

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.

6 participants