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: Breadcrumb filtering react.fragment #18340

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@long-zhuge
Copy link
Contributor

commented Aug 19, 2019

🤔 这个变动的性质是?

  • 新特性提交
  • 日常 bug 修复
  • 站点、文档改进
  • 组件样式改进
  • TypeScript 定义更新
  • 重构
  • 代码风格优化
  • 测试用例
  • 分支合并
  • 其他改动(是关于什么的改动?)

🔗 相关 Issue

close #18260

💡 需求背景和解决方案

  • 问题:动态渲染 Breadcrumb 时,当子组件被 React.Fragment 包裹时,父组件的 separator 会失效,子组件的 separator 会显示默认的 '/' 符号
  • 解决方案:被 React.Fragment 包裹的子项进行打平处理。
<Breadcrumb separator="">
    <React.Fragment>
        <Breadcrumb.Item>当前位置</Breadcrumb.Item>
        <Breadcrumb.Separator>:</Breadcrumb.Separator>,
    </React.Fragment>
</Breadcrumb>

📝 更新日志怎么写?

语言 更新描述
🇨🇳 中文 Breadcrumb组件添加了过滤 Fragment 标签的方法

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

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

@pr-triage pr-triage bot added the PR: unreviewed label Aug 19, 2019

@tests-checker
Copy link

left a comment

Could you please add tests to make sure this change works as expected?

@netlify

This comment has been minimized.

Copy link

commented Aug 19, 2019

Deploy preview for ant-design ready!

Built with commit f08771b

https://deploy-preview-18340--ant-design.netlify.com

@codecov

This comment has been minimized.

Copy link

commented Aug 19, 2019

Codecov Report

Merging #18340 into feature will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           feature   #18340      +/-   ##
===========================================
- Coverage    96.26%   96.22%   -0.04%     
===========================================
  Files          280      280              
  Lines         7527     7533       +6     
  Branches      2094     2095       +1     
===========================================
+ Hits          7246     7249       +3     
- Misses         279      282       +3     
  Partials         2        2
Impacted Files Coverage Δ
components/breadcrumb/Breadcrumb.tsx 96.96% <100%> (+0.3%) ⬆️
components/_util/wave.tsx 85.57% <0%> (-2.89%) ⬇️

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 a5f1fc8...f08771b. Read the comment docs.

1 similar comment
@codecov

This comment has been minimized.

Copy link

commented Aug 19, 2019

Codecov Report

Merging #18340 into feature will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           feature   #18340      +/-   ##
===========================================
- Coverage    96.26%   96.22%   -0.04%     
===========================================
  Files          280      280              
  Lines         7527     7533       +6     
  Branches      2094     2095       +1     
===========================================
+ Hits          7246     7249       +3     
- Misses         279      282       +3     
  Partials         2        2
Impacted Files Coverage Δ
components/breadcrumb/Breadcrumb.tsx 96.96% <100%> (+0.3%) ⬆️
components/_util/wave.tsx 85.57% <0%> (-2.89%) ⬇️

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 a5f1fc8...f08771b. Read the comment docs.

@afc163
afc163 approved these changes Aug 19, 2019
@@ -47,6 +48,16 @@ function defaultItemRender(route: Route, params: any, routes: Route[], paths: st
return isLastItem ? <span>{name}</span> : <a href={`#/${paths.join('/')}`}>{name}</a>;
}

function filterFragment(children: any) {
return toArray(children).map((element: any) => {
if (React.isValidElement(element) && element.type === React.Fragment) {

This comment has been minimized.

Copy link
@afc163

afc163 Aug 19, 2019

Member

React.Fragment 在 React 15 下不存在,最好确认一下有没有问题。

This comment has been minimized.

Copy link
@long-zhuge

long-zhuge Aug 20, 2019

Author Contributor

React.Fragment 标签是在 React v16.2.0 版本中加入的,之前是没有的。如果在项目中使用会报错。antd cli 的 peerDependencies 中 react >= 16.0.0。所以在15以下版本应该不用考虑吧。
本地实测,在16.0.0版本中,使用 react.fragment 后 react 直接就报错了。在 16.2.0 中正常。

This comment has been minimized.

Copy link
@afc163

afc163 Aug 20, 2019

Member

antd 3.x 还在支持 15。

This comment has been minimized.

Copy link
@afc163

afc163 Aug 20, 2019

Member

使用 react.fragment 后 react 直接就报错了

不应该报错吧,顶多这句 React.Fragment 是 undefined,看看会不会有非预期的表现就行。

This comment has been minimized.

Copy link
@long-zhuge

long-zhuge Aug 20, 2019

Author Contributor

在 15 版本中 react-dom 会报错,找不到对应的类/组件。当时官方推荐使用的是 react-addons-create-fragment 插件。
我们现在修复的 bug 会覆盖以往的版本吗?

This comment has been minimized.

Copy link
@afc163

afc163 Aug 20, 2019

Member

React 15 下肯定是不能直接用 React.Fragment 的。

我的意思是这段代码不要影响现有 React 15 的项目(即使项目里没有用 <></>)。

This comment has been minimized.

Copy link
@long-zhuge

long-zhuge Aug 20, 2019

Author Contributor

嗯啊,我打包了测试下看看。

This comment has been minimized.

Copy link
@long-zhuge

long-zhuge Aug 20, 2019

Author Contributor

本地测试 react v15.6.0 版本可以正常使用。
demo 下的用例也回归了一遍,都能正常使用。

This comment has been minimized.

Copy link
@long-zhuge

long-zhuge Aug 20, 2019

Author Contributor

image

@afc163
Copy link
Member

left a comment

React.Fragment 在 React 15 下不存在,最好实测确认一下有没有问题。

@long-zhuge

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

React.Fragment 在 React 15 下不存在,最好实测确认一下有没有问题。

好的,我实测下是否会有问题。

@afc163
afc163 approved these changes Aug 20, 2019

@afc163 afc163 merged commit f9f6e8d into ant-design:feature Aug 20, 2019

25 of 27 checks passed

Header rules - ant-design No header rules processed
Details
Pages changed - ant-design 219 new files uploaded
Details
DEP All dependencies are resolved
LGTM analysis: JavaScript No new or fixed alerts
Details
Mixed content - ant-design No mixed content detected
Details
Redirect rules - ant-design 18 redirect rules processed
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
ant-design.ant-design #ant design succeeded
Details
ci/circleci: compile Your tests passed on CircleCI!
Details
ci/circleci: dist Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_dist Your tests passed on CircleCI!
Details
ci/circleci: test_dist_15 Your tests passed on CircleCI!
Details
ci/circleci: test_dom Your tests passed on CircleCI!
Details
ci/circleci: test_dom_15 Your tests passed on CircleCI!
Details
ci/circleci: test_es Your tests passed on CircleCI!
Details
ci/circleci: test_es_15 Your tests passed on CircleCI!
Details
ci/circleci: test_lib Your tests passed on CircleCI!
Details
ci/circleci: test_lib_15 Your tests passed on CircleCI!
Details
ci/circleci: test_node Your tests passed on CircleCI!
Details
ci/circleci: test_node_15 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.26%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +3.73% compared to a5f1fc8
Details
netlify/ant-design/deploy-preview Deploy preview ready!
Details
security/snyk - package.json (paranoidjk) No manifest changes detected

@long-zhuge long-zhuge deleted the long-zhuge:feature branch Aug 29, 2019

@chenshuai2144 chenshuai2144 referenced this pull request Sep 2, 2019
5 of 14 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.