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 inline collapsed show without icon #24330

Merged
merged 14 commits into from
May 27, 2020
Merged

fix: menu inline collapsed show without icon #24330

merged 14 commits into from
May 27, 2020

Conversation

xrkffgg
Copy link
Member

@xrkffgg xrkffgg commented May 20, 2020

[中文版模板 / Chinese template]

🤔 This is a ...

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

🔗 Related issue link

close #24311

💡 Background and solution

  • Use Avatar component to automatically adjust the font size of content

Rendering

image

📝 Changelog

Language Changelog
🇺🇸 English Adjust text shows the first character when Menu is collapsed in inline mode.
🇨🇳 Chinese 调整 Menu inline 模式下未设置 icon 的菜单收起时文字显示第一个字符。

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • 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

View rendered components/menu/demo/inline-collapsed.md

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

@ant-design-bot
Copy link
Contributor

ant-design-bot commented May 20, 2020

@xrkffgg
Copy link
Member Author

xrkffgg commented May 20, 2020

scale: nodeWidth - 8 < childrenWidth ? (nodeWidth - 8) / childrenWidth : 1,

Avatar 默认两边各留了 4px 的边界,计划增加一个 API 可以控制这个值

这样的话,在这个地方 会更完善一些

@afc163

what do you think?

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #24330 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24330   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files         363      363           
  Lines        7230     7234    +4     
  Branches     1966     2009   +43     
=======================================
+ Hits         7165     7169    +4     
  Misses         65       65           
Impacted Files Coverage Δ
components/menu/MenuItem.tsx 96.87% <100.00%> (+0.20%) ⬆️
components/menu/SubMenu.tsx 94.73% <100.00%> (+0.61%) ⬆️

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 89d7101...1a17a7e. Read the comment docs.

@xrkffgg
Copy link
Member Author

xrkffgg commented May 21, 2020

/rebase

@afc163
Copy link
Member

afc163 commented May 21, 2020

Avatar 默认两边各留了 4px 的边界,计划增加一个 API 可以控制这个值

可行,不过就要发到 feature 分支了。

@afc163
Copy link
Member

afc163 commented May 21, 2020

image

文字一多就没法读,试着拿第一个字符吧。

@@ -1,5 +1,6 @@
@import '../../style/themes/index';
@import '../../style/mixins/index';
@import '../../avatar/index';
Copy link
Member

Choose a reason for hiding this comment

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

样式依赖要放在 style/index.ts 中。

@xrkffgg
Copy link
Member Author

xrkffgg commented May 21, 2020

dark

image

light

image

@xrkffgg
Copy link
Member Author

xrkffgg commented May 21, 2020

😂😂😂 如果是第一个字符,完全没必要 Avatar 了啊

@afc163
Copy link
Member

afc163 commented May 21, 2020

带个圈当背景,图形整体性会强很多。

@afc163
Copy link
Member

afc163 commented May 21, 2020

一个 N/O 感觉像是乱码。

@xrkffgg
Copy link
Member Author

xrkffgg commented May 21, 2020

image

@xrkffgg
Copy link
Member Author

xrkffgg commented May 21, 2020

我觉得有 必要 确认下要用哪种方式 😂😂

如果带圈,圈的的大小,边框色,背景色都需权衡下

@xrkffgg
Copy link
Member Author

xrkffgg commented May 21, 2020

image

@yoyo837
Copy link
Contributor

yoyo837 commented May 21, 2020

这种时候该视觉的同志勇敢的站出来说话了

@xrkffgg xrkffgg mentioned this pull request May 21, 2020
17 tasks
@zombieJ
Copy link
Member

zombieJ commented May 21, 2020

这么看似乎都不用 Avatar 了,直接 children 第一个字母就成了。

@afc163
Copy link
Member

afc163 commented May 22, 2020

那就首字母吧,不用 Avatar 了。

@xrkffgg
Copy link
Member Author

xrkffgg commented May 23, 2020

那就首字母吧,不用 Avatar 了。

首字母的话,需要带圈吗?实心圈 还是 光有边框的?

@hengkx
Copy link
Member

hengkx commented May 23, 2020

首字母 一样 岂不是很尴尬

@ant-design-bot
Copy link
Contributor

UI Testing

@xrkffgg xrkffgg changed the title fix: menu inline collapsed show without icon wip fix: menu inline collapsed show without icon May 27, 2020
@ant-design-bot
Copy link
Contributor

@xrkffgg xrkffgg changed the title wip fix: menu inline collapsed show without icon fix: menu inline collapsed show without icon May 27, 2020
// inline-collapsed.md demo 依赖 span 来隐藏文字,有 icon 属性,则内部包裹一个 span
// ref: https://github.com/ant-design/ant-design/pull/23456
if (!icon || (isValidElement(children) && children.type === 'span')) {
if (children && inlineCollapsed && level === 1 && typeof children === 'string') {
return <p>{children.charAt(0)}</p>;
}
return children;
Copy link
Member Author

@xrkffgg xrkffgg May 27, 2020

Choose a reason for hiding this comment

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

其实 children 为空的话,要不要 span 都行。
换了种方式,保持原有 snap 。

Copy link
Member

Choose a reason for hiding this comment

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

为啥是 p ?

Copy link
Member

Choose a reason for hiding this comment

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

用 span 加 className 呢。

Copy link
Member Author

Choose a reason for hiding this comment

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

试了 span ,不太行。换成了 div

@ant-design-bot
Copy link
Contributor

UI Testing

@ant-design-bot
Copy link
Contributor

@xrkffgg
Copy link
Member Author

xrkffgg commented May 27, 2020 via email

@xrkffgg
Copy link
Member Author

xrkffgg commented May 27, 2020 via email

@ant-design-bot
Copy link
Contributor

UI Testing

@ant-design-bot
Copy link
Contributor

@afc163 afc163 merged commit 9b54bd7 into master May 27, 2020
@afc163 afc163 deleted the fix-menu branch May 27, 2020 14:19
@zombieJ
Copy link
Member

zombieJ commented May 27, 2020

@shaodahong,用更新的方式更新 UI test 评论,否则留言太多了。

@shaodahong
Copy link
Member

@shaodahong,用更新的方式更新 UI test 评论,否则留言太多了。

嗯。是这么做的,但是 rebase 后的评论就拉不到了……

@shaodahong
Copy link
Member

fetch https://api.github.com/repos/ant-design/ant-design/issues/24330/comments

@ant-design ant-design deleted a comment from ant-design-bot May 28, 2020
@zombieJ
Copy link
Member

zombieJ commented May 28, 2020

嗯。是这么做的,但是 rebase 后的评论就拉不到了……

神奇……

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 inlineCollapsed without icon display Error
9 participants