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(typography): menu item title cannot be centered vertically #41146

Merged
merged 2 commits into from Oct 8, 2023

Conversation

Yuiai01
Copy link
Contributor

@Yuiai01 Yuiai01 commented Mar 9, 2023

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Workflow
  • Other (about what?)

🔗 Related issue link

close #41143

💡 Background and solution

由于 Typography 设置 ellipsis 时会默认添加 vertical-align: bottom,且 Typography 具有默认行高,所以会无法垂直居中,现将 Menu.Item 透传,使其能垂直居中,以下是修复前后对比:
修复前:
image
修复后:
image

📝 Changelog

Language Changelog
🇺🇸 English Fixed Menu.Item nested Typography failing to center vertically when ellipsis is true
🇨🇳 Chinese 修复 Menu.Item 嵌套的 Typography 其 ellipsis 为 true 时无法垂直居中

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (f42d066) 100.00% compared to head (6bb729a) 100.00%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master    #41146   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          681       681           
  Lines        11522     11522           
  Branches      3097      3097           
=========================================
  Hits         11522     11522           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@afc163
Copy link
Member

afc163 commented Mar 9, 2023

不应该删默认 line-height,应该增加 menu item 的 line-height 的样式选择器优先级。

@Yuiai01
Copy link
Contributor Author

Yuiai01 commented Mar 9, 2023

不应该删默认 line-height,应该增加 menu item 的 line-height 的样式选择器优先级。

嗯嗯,但是目前由于 typography 存在默认 line-height 导致很多嵌套在 typography 内部的文本无法保持垂直居中,提升 menu item 其 line-height 的优先级是可以解决关联问题,但是后续可能还会出现类似的问题

@afc163
Copy link
Member

afc163 commented Mar 11, 2023

typography 默认 line-height 是合理的,否则文本段行高不对。

@afc163
Copy link
Member

afc163 commented Mar 11, 2023

@Yuiai01
Copy link
Contributor Author

Yuiai01 commented Mar 11, 2023

typography 默认 line-height 是合理的,否则文本段行高不对。

嗯嗯

@Yuiai01
Copy link
Contributor Author

Yuiai01 commented Mar 11, 2023

可以看看 https://app.argos-ci.com/ant-design/ant-design/builds/6664/40674977

这个当初是想看下影响大不大,但现在看来只能从应用场景来解决问题了

@@ -303,6 +303,10 @@ const getBaseStyle: GenerateStyle<MenuToken> = (token) => {

[`${componentCls}-title-content`]: {
transition: `color ${motionDurationSlow}`,

'> *': {
Copy link
Member

Choose a reason for hiding this comment

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

还是感觉这样有点耦合,内部元素万一就是需要这个 lineHeight,这么改就破坏了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

还是感觉这样有点耦合,内部元素万一就是需要这个 lineHeight,这么改就破坏了

嗯嗯,那我这边再尝试修改一下~

@zombieJ
Copy link
Member

zombieJ commented Mar 13, 2023

https://codesandbox.io/s/ji-ben-shi-yong-antd-5-2-2-forked-90t2m9?file=/demo.js

这样呢?我觉得尽量只影响到自己更好一些。

@MadCcc
Copy link
Member

MadCcc commented Mar 13, 2023

这个 line-height 之前加上是因为缺少了全局的 line-height (v4 的全局样式)。
现在 image test 都包裹了 App 组件,所以 argos 也正常。
从 Typo 组件角度讲确实是需要 line-height 的。

Copy link

@AbdullahWins AbdullahWins left a comment

Choose a reason for hiding this comment

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

good job!

@Yuiai01
Copy link
Contributor Author

Yuiai01 commented Aug 23, 2023

good job!

The approach of fixing this is destructive, so it's necessary to consider a better solution here, or reset the styles locally.

@Yuiai01 Yuiai01 changed the title fix(typography): fix nested typography could not be centered vertically fix(typography): menu item title cannot be centered vertically Oct 7, 2023
@argos-ci
Copy link

argos-ci bot commented Oct 7, 2023

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) 👍 Changes approved 1 change Oct 7, 2023, 11:30 AM

@afc163 afc163 merged commit b57dc7a into ant-design:master Oct 8, 2023
51 checks passed
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.

menu item break with ellipsis typography text
7 participants