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.

Sign up for GitHub

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

Jump to bottom

fix: 修复表格搜索条件按钮没有宽度的bug #138

Open
wants to merge 6 commits into
base: feature
Choose a base branch
from
Open

fix: 修复表格搜索条件按钮没有宽度的bug #138

wants to merge 6 commits into from

Conversation

710831265
Copy link

@710831265 710831265 commented Apr 6, 2021 •
edited
Loading

PR 类型：

  • 新功能 Feat
  • 缺陷修复 Bug
  • 文档改进 Doc
  • 添加测试用例 Test
  • 性能优化 Perfs
  • 重构 Refactor or Chore

❗️ 请务必确保：

  • ci checkers钩子执行正确✅
  • 代码经历过书写-构建-使用自测流程
  • 有描述清晰的组件文档
  • TS 类型定义精准，且无恶意 any

请尽量保证：

@vercel
Copy link

vercel bot commented Apr 6, 2021 •
edited
Loading

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/juicecube/mlz-admin/7UQ1vNHAjeQhxWVnBM7PBP3oKhzc
✅ Preview: https://mlz-admin-git-fork-710831265-hotfix-table-search-juicecube.vercel.app

@codecov-io
Copy link

codecov-io commented Apr 6, 2021 •
edited
Loading

Codecov Report

Merging #138 (502509b) into feature (7c2876d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           feature     #138   +/-   ##
========================================
  Coverage    89.38%   89.38%           
========================================
  Files           18       18           
  Lines          358      358           
  Branches       137      137           
========================================
  Hits           320      320           
  Misses          36       36           
  Partials         2        2
Impacted Files Coverage Δ
...ckages/mlz-admin/src/table/common-search/index.tsx 96.20% <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 7c2876d...502509b. Read the comment docs.

@milobluebell
Copy link
Member

  1. 还有一点小问题，也看下：
    image
    image

linkage: https://mlz-admin-git-fork-710831265-hotfix-table-search-juicecube.vercel.app/components/interactivity/table#%E5%B8%A6%E5%8F%AF%E6%94%B6%E8%B5%B7%E5%B1%95%E5%BC%80%E7%9A%84%E6%90%9C%E7%B4%A2%E9%A1%B9

  1. 看下review评论

@710831265

milobluebell
milobluebell reviewed Apr 6, 2021
View reviewed changes
packages/mlz-admin/src/table/common-search/index.tsx Outdated
@@ -98,7 +98,7 @@ const InternalCommonSearch = (props: ICommonSearch<unknown>) => {
const collapsedHandler = () => toggleCollapsed(!collapsed);

const formSubmitters = searchings.length ? (
<Col span={sparedColSpan < perColspan + collapsingButtonColspan ? 24 : sparedColSpan} style={{ textAlign: 'right', marginBottom: hasMoreInteractionArea ? 16 : 0 }} flex="1">
<Col span={sparedColSpan < perColspan + collapsingButtonColspan ? 24 : sparedColSpan} style={{ textAlign: 'right', marginBottom: hasMoreInteractionArea ? 16 : 0, flex: '1 1 auto' }}>
Copy link
Member

Choose a reason for hiding this comment

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

这里是不能使用flex属性吗？

这样允许的话，尽量使用antd暴露出来的属性。

Copy link
Member

Choose a reason for hiding this comment

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

<Col span={sparedColSpan < perColspan + collapsingButtonColspan ? 24 : sparedColSpan} style={{ textAlign: 'right', marginBottom: hasMoreInteractionArea ? 16 : 0 }} flex="1 1 auto">

Copy link
Author

Choose a reason for hiding this comment

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

我在chrome浏览器里面，看到flex: 1 === flex: 1 1 0%，而不是flex: 1 1 auto，
所以当宽度不够时，按钮组所属的栅格宽度会变成0。
参考这篇文章：https://zhuanlan.zhihu.com/p/136223806

Copy link
Member

@milobluebell milobluebell Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

好，我懂你这个意思。我意思是这个flex，使用组件的flex属性传递，而不是style传递，试一下可不可以：

<A style="ABCD" flex="1 1 auto"/>

而不是

<A style="ABCD,flex: `1 1 auto`"/>

别直接套style，容易出问题，它内部可能会有用。使用antd的属性。 其它地方也一样，如果看到的话都改一下。

Copy link
Member

@milobluebell milobluebell Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

我在chrome浏览器里面，看到flex: 1 === flex: 1 1 0%，而不是flex: 1 1 auto，
所以当宽度不够时，按钮组所属的栅格宽度会变成0。
参考这篇文章：https://zhuanlan.zhihu.com/p/136223806

flex-basis的默认值是auto，但是在flex附和属性里会被修改成0px or 0%。 这个不错，学到了。

https://developer.mozilla.org/zh-CN/docs/Web/CSS/flex-basis#%E5%8F%96%E5%80%BC
https://www.bilibili.com/read/cv9314814/

milobluebell
milobluebell approved these changes Apr 6, 2021
View reviewed changes
Copy link
Member

@milobluebell milobluebell left a comment

Choose a reason for hiding this comment

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

先approve

@milobluebell
Copy link
Member

https://pipelines.actions.githubusercontent.com/WRkmHu3qpoml3Oe8XPpbQZjMpgx0Rjb5hUkd93BwPMJhGPkI6X/_apis/pipelines/1/runs/253/signedlogcontent/22?urlExpires=2021-04-09T13%3A54%3A55.9661048Z&urlSigningMethod=HMACV1&urlSignature=fxsMAwUUQ%2BxTF17WEjTKvytLG%2BB1wh2t95TFDPyAeIQ%3D ← 单元测试没通过。

c9dd9bb ← dom结构发生变化，需要更新一下快照：npm run test:update，否则快照测试通不过。

之后，本地运行下npm run test，看下还有哪些测试没通过的，可能需要改单元测试代码。

@710831265 都熟悉了就不麻烦了😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milobluebell milobluebell milobluebell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@710831265 @codecov-io @milobluebell