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

Add Radio.Group props optionType to choose item type #12852

Closed
wants to merge 1 commit into from

Conversation

xpol
Copy link

@xpol xpol commented Oct 27, 2018

Add Radio.Group props optionType to choose item type (Radio or Radio.Button) when options is used.

optionType

  • radioforRadio` (default, current behavior)
  • button for Radio.Button

First of all, thank you for your contribution! :-)

Please makes sure that these checkboxes are checked before submitting your PR, thank you!

  • Make sure that you propose PR to right branch: bugfix for master, feature for branch feature.

  • Make sure that you follow antd's code convention.

  • Run npm run lint and fix those errors before submitting in order to keep consistent code style.

  • Rebase before creating a PR to keep commit history clear.

  • Add some descriptions and refer relative issues for you PR.

  • Update API docs for the component.

  • Update/Add demo to demonstrate new feature.

  • Update TypeScript definition for the component.

  • Add unit tests for the feature.

@netlify
Copy link

netlify bot commented Oct 27, 2018

Deploy preview for ant-design ready!

Built with commit c8f6c4e

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

…o.Button) when `options` is used.

`optionType` default to `radio` for `Radio` (current behavior), and set to `button` for `Radio.Button`
@codecov
Copy link

codecov bot commented Oct 27, 2018

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff             @@
##           feature   #12852      +/-   ##
===========================================
- Coverage    92.64%    92.6%   -0.04%     
===========================================
  Files          215      215              
  Lines         5613     5614       +1     
  Branches      1598     1571      -27     
===========================================
- Hits          5200     5199       -1     
- Misses         407      411       +4     
+ Partials         6        4       -2
Impacted Files Coverage Δ
components/radio/group.tsx 90.38% <100%> (+0.18%) ⬆️
components/upload/UploadList.tsx 88.46% <0%> (-1.93%) ⬇️
components/transfer/index.tsx 89.8% <0%> (ø) ⬆️
components/time-picker/index.tsx 81.96% <0%> (ø) ⬆️

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 ba5409a...c8f6c4e. Read the comment docs.

@zombieJ
Copy link
Member

zombieJ commented Oct 29, 2018

If we use optionType, we can remove RadioButton in doc. @afc163 @yesmeck

@yesmeck
Copy link
Member

yesmeck commented Oct 30, 2018

没什么特别的好处,也没解决什么问题,没必要换一种方式吧。

@xpol
Copy link
Author

xpol commented Oct 30, 2018

用声明式的方式可以通过声明不同的子元素来选择是 Radio 还是 RadioButton

<Radio.Group>
  <Radio>...</Radio>
<Radio.Group>

or 

<Radio.Group>
  <Radio.Button>...</Radio.Button>
<Radio.Group>

然而通过 options 的方式,就强制选择 Radio 。这就是API设计上的不对称的地方。

如果没有 optionType 选项来实现,那么用户自己来实现这个需要再处理一遍 string类型的 options 和 object 类型的 options,然而这些都是 antd 内部的实现细节:

export default class RadioButtonGroup extends PureComponent {
  render() {
    const {options, value, onChange} = this.props
    return (
      <Radio.Group value={value} onChange={onChange}>
        {options.map((option, index) => {
          if (typeof option === 'string') {
            return (
              <Radio.Button key={index} value={option}>
                {option}
              </Radio.Button>
            )
          } else {
            return (
              <Radio.Button key={index} value={option.value}>
                {option.label}
              </Radio.Button>
            )
          }
        })}
      </Radio.Group>
    )
  }
}

如果将来 options 的格式有变化,用户还得跟着改一遍。

这个PR就是解决这个问题的。

不知道这样解释是否能改变你的判断 @yesmeck

@yesmeck
Copy link
Member

yesmeck commented Oct 30, 2018

之前没仔细看,这样是可以的,两种方式都保留好了。

@yesmeck
Copy link
Member

yesmeck commented Oct 30, 2018

@zombieJ
Copy link
Member

zombieJ commented Oct 30, 2018

文档里去掉 Radio.Button 的例子,只留下 optionType 就行了。用户没必要知道两种写法。

还是不加了吧,加了反而会让用户疑惑:

<Group optionType="button">
  <Radio ... />
</Group>

原本的虽然麻烦,但是是可以实现这个功能的。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants