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

fix(menu): solve the problem of display being cropped by not enabling sliding in popup mode #2057 #2129

Closed
wants to merge 1 commit into from

Conversation

PBK-B
Copy link
Contributor

@PBK-B PBK-B commented Apr 6, 2023

🤔 这个 PR 的性质是?

  • 日常 bug 修复
  • 新特性提交
  • 文档改进
  • 演示代码改进
  • 组件样式/交互改进
  • CI/CD 改进
  • 重构
  • 代码风格优化
  • 测试用例
  • 分支合并
  • 其他

🔗 相关 Issue

#2057

💡 需求背景和解决方案

@MingjieZhang @andyjxli
具体原因:
https://github.com/Tencent/tdesign-react/blob/develop/src/menu/Menu.tsx#L44 处判断设置 collapsed 时会添加 t-menu--scroll 样式,导致子组件无法溢出

解决方案1:
通过判断 expandType 是否为 popup 做为条件来判断是否添加 t-menu--scroll 样式。(Vue 那边就是这样处理的 相关代码 不过我觉得治标不治本,且会导致 menu 内容高度超过之后不可滑动)
方案1修复代码: PBK-B@6ce8d8e

解决方案2:
将 SubMenu 中的 popup 部分节点直接挂载到 body 上,或者在 SubMenu 中直接使用 Popup 组件(两种做法实际上是一样的解决思路),但是这样的话应该是需要在 tdesign-common 中修改 t-menu__popup 相关样式了,我下面的代码在 theme="dark" 情况下还是白色样式就是这样
方案2修复代码: PBK-B@0614e05

📝 更新日志

  • fix(menu): 修复 expandType 属性设置 popup 时样式异常问题

  • 本条 PR 不需要纳入 Changelog

☑️ 请求合并前的自查清单

⚠️ 请自检并全部勾选全部选项⚠️

  • 文档已补充或无须补充
  • 代码演示已提供或无须提供
  • TypeScript 定义已补充或无须补充
  • Changelog 已提供或无须提供

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

完成

@xiaosansiji
Copy link
Collaborator

倾向于方案2,vue-next 已经改为基于 Popup 实现了,长久看所有弹出类的需求都由独立的 Popup 处理是合理的,现在相当于在 menu 中独立用样式实现了弹窗,vue2 和 react 版本最好都按引入 Popup 的方式来实现 @PBK-B

@PBK-B
Copy link
Contributor Author

PBK-B commented Apr 16, 2023

倾向于方案2,vue-next 已经改为基于 Popup 实现了,长久看所有弹出类的需求都由独立的 Popup 处理是合理的,现在相当于在 menu 中独立用样式实现了弹窗,vue2 和 react 版本最好都按引入 Popup 的方式来实现 @PBK-B

@xiaosansiji 好的,我已经提交 plan2 了,并且参考 vue-next 修复了 theme 问题。重新提交 PR (#2147) 了

@PBK-B PBK-B closed this Apr 16, 2023
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.

2 participants