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: icon innerNode render #18592

Merged
merged 8 commits into from Sep 5, 2019

Conversation

@kanweiwei
Copy link
Contributor

commented Sep 1, 2019

🤔 This is a ...

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

🔗 Related issue link

#17962

💡 Background and solution

Background:Icon组件并没有如文档里写的,compoent和type同时使用的话,type会失效,实际起作用的还是type。因为实际代码里,innerNode被反复修改,compoent、children、type,按顺序判断,只要后面的有值,都会覆盖前面的。
https://codesandbox.io/s/antd-reproduction-template-08w2u

另外文档里component 的props,英文文档里default,实际代码里是写死的,传了也不起作用,没有往component上传递。

solution: component、children、type, 按顺序判断,哪个有值,渲染时就直接返回;修改英文文档

📝 Changelog

Language Changelog
🇺🇸 English Fix rendering priority issue that props component and children is lower proiorty than props type in the Icon component
🇨🇳 Chinese 修复 Icon 组件中 component 和 children 属性优先级低于 type 的问题

☑️ Self Check before Merge

  • 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

@pr-triage pr-triage bot added the PR: unreviewed label Sep 1, 2019

@netlify

This comment has been minimized.

Copy link

commented Sep 1, 2019

Deploy preview for ant-design ready!

Built with commit f793b91

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

kwwnjujlc added 3 commits Sep 1, 2019

@afc163 afc163 requested review from zombieJ and vagusX Sep 1, 2019

@afc163

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

Would you mind fill the form?

图片

@kanweiwei

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2019

@afc163
好了

@codecov

This comment has been minimized.

Copy link

commented Sep 1, 2019

Codecov Report

Merging #18592 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18592      +/-   ##
==========================================
+ Coverage   96.73%   96.73%   +<.01%     
==========================================
  Files         280      280              
  Lines        7537     7538       +1     
  Branches     2102     2057      -45     
==========================================
+ Hits         7291     7292       +1     
  Misses        244      244              
  Partials        2        2
Impacted Files Coverage Δ
components/icon/index.tsx 88% <100%> (+0.24%) ⬆️

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 a111e62...f793b91. Read the comment docs.

@afc163

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

修改Icon组件里的渲染判断逻辑

对用户使用时有什么影响?

@kanweiwei

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2019

对用户使用时有什么影响?

image
1.现有的渲染优先级,和这个文档说明不符。同时使用component和type时,实际是type覆盖了component。现有的优先级是type> children>component。我改了以后是component > children>type。

2.另外一个问题,component的props没有作用,现有代码是写死的,中文文档写的是 只读值,但是英文文档内写的 Default,造成#17962 那位外国朋友以为,传width、height、fill等参数,在自定义的component组件里可以获取到外部传过来的值。其实不是这样,实际width、height、fill等参数都是写死的,比如你外面写了<Icon component={CustomIconComponent} width={'30px'} />, component 接收到prop.width 一直是'1em'。
我建议这个地方要改一下,不然文档告诉我这里传进来的是个写死的值,又改不了,感觉这个功能说明对用户来说没意义。

@afc163

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

changelog 要写对用户使用端的变化和影响,而不是描述我们的修改。

@vagusX

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

@HeskeyBaozi 怎么看

@HeskeyBaozi

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

@vagusX

@HeskeyBaozi 怎么看

我记得我的逻辑的确是优先级 component > Children > type。而且有注释

// component > children > type

我分析一下哪里改了。
c4b6daf#diff-f309b6ba00263748dbcb1cb596e3b241R78-R94
找到了,是这个 commit 变成本来 return 变为 innerNode 变量更改来判断渲染节点逻辑。后续我当时没有改回来(sad。所以 <=3.9.0 都是正确的渲染优先级, >=3.9.1 就不正确了。

关于文档,的确英文文档需要从default改为readonly.
并且可能需要额外说明这些属性的作用和为什么是只读的。

作用主要是:

  • 穿透一些使得SVG图标可像文字一样处理的样式,如height: 1emwidth: 1emfill: currentColor
  • 用户可以通过这些属性对自己的自定义组件实现一些逻辑。
    Edit antd reproduction template 这个例子通过判断由spin属性带来的anticon-spin样式名实现了一个“新的加载样式”。

传入的属性由父级Icon决定

const innerSvgProps: CustomIconComponentProps = {
...svgBaseProps,
className: svgClassString,
style: svgStyle,
viewBox,
};

这个pr里优先级改过来了可以approved。文档里我觉得可以加上一段注解说明告知用户这些都是穿透属性。
不建议 component 组件通过 Icon 传入额外属性作为 component 组件的属性,这些属性和外部逻辑由 component 组件本身确定即可。

@kanweiwei

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

虽然不清楚其他用户是怎么用Icon组件的,但我觉得这个穿透属性写到文档里意义不大,如果用户在使用Icon 的 component自定义组件,可能完全不理会这个props,因为这个props不受用户的控制,哪天传进来的className变了,用户不注意antd的changelog的话,基本不会注意以前的代码在这里需要调整。

@HeskeyBaozi

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

@kanweiwei
事实上自定义 component 的组件获取到的属性,和使用官方图标时内部渲染的 svg 元素接受的属性时一致的。可以理解为当用户的自定义组件想保持与 ant-design 官方处理图标的样式等,可以通过扩展符号...方便传递,这也是当初我这样写的原因。

const Component = props => <svg {...props} otherProp={anythingElse}>...</svg>;

至于文档,我觉得对于这种相对于用户可选的一些代码技巧,用户最好还是知道其存在,至于是否使用由用户决定即可。文档中的说明最好是说清楚用意和会用户代码的影响(同上面写changelog的原则),而不是删去。

@kanweiwei

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

@HeskeyBaozi 👌,可以理解。那文档部分和changelog怎么改,我不太擅长写这个。

@vagusX

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@vagusX

@HeskeyBaozi 怎么看

我记得我的逻辑的确是优先级 component > Children > type。而且有注释

// component > children > type

我分析一下哪里改了。
c4b6daf#diff-f309b6ba00263748dbcb1cb596e3b241R78-R94
找到了,是这个 commit 变成本来 return 变为 innerNode 变量更改来判断渲染节点逻辑。后续我当时没有改回来(sad。所以 <=3.9.0 都是正确的渲染优先级, >=3.9.1 就不正确了。

4.0 里面已经没有 type 的优先级了,只需要找回 component > children 的关系即可,4.0 版本这个改动是否有必要找回来呢 cc @afc163 @zombieJ

@afc163

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

那文档部分和changelog怎么改,我不太擅长写这个。
@vagusX 帮忙总结一下?

@vagusX

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

那文档部分和changelog怎么改,我不太擅长写这个。
@vagusX 帮忙总结一下?

@afc163 文档上可能需要说明下 component > children > type 的优先级的问题,提示用户如果出现混用的情况,需要明确下用法

@vagusX

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@kanweiwei 给你的分支发了 mr,增加了文档描述 https://github.com/kanweiwei/ant-design/pull/1,上面的 changelog 也更新了

cc @afc163

@buildsize

This comment has been minimized.

Copy link

commented Sep 4, 2019

File name Previous Size New Size Change
package-lock.json 847.74 KB 849.74 KB 2 KB (0%)
@vagusX

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@kanweiwei 我又更新了下,麻烦在合并下哈 kanweiwei#2

@vagusX
vagusX approved these changes Sep 4, 2019

@afc163 afc163 merged commit e479f1f into ant-design:master Sep 5, 2019

25 of 27 checks passed

Header rules - ant-design No header rules processed
Details
Pages changed - ant-design 215 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
buildsize Significant change of package-lock.json up by 2 KB (0.24%)
Details
ci/circleci: check_metadata Your tests passed on CircleCI!
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
netlify/ant-design/deploy-preview Deploy preview ready!
Details
security/snyk - package.json (paranoidjk) No new issues
Details

@pr-triage pr-triage bot added PR: merged and removed PR: reviewed-approved labels Sep 5, 2019

@afc163

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

辛苦各位~

@ZengMX

This comment has been minimized.

Copy link

commented Sep 11, 2019

@afc163
自从3.9.0这个版本以后
我的这个问题就开始出现,请问这个需要怎么配置?
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.