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(comp:*): the control type of all form components is incomplete #1495

Merged
merged 1 commit into from
Mar 8, 2023
+87 −38
Merged

fix(comp:*): the control type of all form components is incomplete #1495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/components/cascader/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `v-model:value` | 当前选中的的值 | `any \| any[] \| any[][]` | - | - | 使用 `control` 时，此配置无效 |
| `v-model:expandedKeys` | 展开节点的 `key` 数组 | `VKey[]` | - | - | - |
| `v-model:loadedKeys` | 已经加载完毕的节点的 `key` | `VKey[]` | - | - | - |
Copy link

Choose a reason for hiding this comment

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

review

  1. control type: the patch changes the type of control to string | number | (string | number)[] | AbstractControl, which adds a few possible value that can be passed in. This should not cause any bug risk, and might be more intuitive to use.
  2. For v-model:value, the type changed from any | any[] | any[][] to any | any[] | any[][], which is a minor change but should not affect the behavior of the code.
  3. For v-model:expandedKeys, the type changed from VKey[] to VKey[], which does not change the code's behavior.
  4. For v-model:loadedKeys, the type changed from VKey[] to VKey[], which does not change the code's behavior.

In conclusion, the patch does not appear to introduce any bug risks, and might make the code easier to use.

Expand Down
5 changes: 4 additions & 1 deletion packages/components/cascader/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ export const cascaderPanelProps = {
} as const

export const cascaderProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
value: { type: null, default: undefined },
expandedKeys: { type: Array as PropType<VKey[]>, default: undefined },
loadedKeys: { type: Array as PropType<VKey[]>, default: undefined },
Copy link

Choose a reason for hiding this comment

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

  1. The code looks good, no bug risks or issues seen here.

  2. The only suggestion I have is to add a comment above the code to help explain what it is doing and why it is being added. This will help future developers understand the code more easily.

Expand Down
4 changes: 2 additions & 2 deletions packages/components/checkbox/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `v-model:checked` | 指定当前勾选框是否选中 | `boolean \| string \| number` | - | - | 使用 `control` 时，此配置无效 |
| `autofocus` | 是否以自动聚焦 | `boolean` | `false` | - | - |.
| `buttoned` | 是否以按钮显示 | `boolean` | - | - | - |
Expand Down Expand Up @@ -36,7 +36,7 @@
| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `v-model:value` | 指定当前勾选框是否选中 | `any[]` | - | - | 使用 `control` 时，此配置无效 |
| `buttoned` | 设置组内 `IxCheckbox` 的 `buttoned` 属性 | `boolean` | `false` | - | - |
| `dataSource` | 勾选框组数据源 | `CheckboxData[]` | - | - | 优先级高于 `default` 插槽 |
Copy link

Choose a reason for hiding this comment

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

the code review.

The patch looks good and it seems like the type of control has been changed to include string, number, or an array of strings and numbers. It also looks like the v-model attribute has been updated to accept any array type.

A few things that I would suggest is to add some comments in the code to explain why the changes have been made, what the purpose of this change is, and how it improves the existing code. This will help future developers understand the code better. Additionally, it would be helpful to run some tests to ensure that the code works as intended.

Overall, the patch looks good and the update should improve the existing code.

Expand Down
10 changes: 8 additions & 2 deletions packages/components/checkbox/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ import type { SpaceProps } from '@idux/components/space'
import type { DefineComponent, HTMLAttributes, LabelHTMLAttributes, PropType } from 'vue'

export const checkboxProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
checked: { type: [String, Number, Boolean] as PropType<CheckValue>, default: undefined },

autofocus: { type: Boolean, default: false },
Expand Down Expand Up @@ -50,7 +53,10 @@ export type CheckboxComponent = DefineComponent<
export type CheckboxInstance = InstanceType<DefineComponent<CheckboxProps, CheckboxBindings>>

export const checkboxGroupProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
value: { type: Array, default: undefined },

buttoned: { type: Boolean, default: false },
Copy link

Choose a reason for hiding this comment

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

the review.

  1. In both the checkboxProps and checkboxGroupProps, the control property has been updated to accept an array of strings, numbers, or objects. This is a good thing as it allows for more flexibility and prevents the need for multiple props when handling multiple values.

  2. There is no validation or type checking being done on the values in the control prop. It is possible that a user could pass in invalid data which could lead to errors. It would be best to add some validation to make sure that only valid values are accepted.

  3. The autofocus prop is not being used anywhere. It would be good to either remove it or make use of it.

  4. The value prop in checkboxGroupProps is not being used anywhere. It would be good to either remove it or make use of it.

Overall, the code looks good. Adding validation and making use of the unused props would improve it further.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/date-picker/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `v-model:open` | 日期面板是否展开 | `boolean` | - | - | - |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `cellTooltip` | 日期节点的tooltip | `(cell: { value: Date, disabled: boolean }) => string | void` | - | - | - |
| `allowInput` | 允许输入模式 | `boolean \| 'overlay'` | `false` | - | `'overlay'` 时在浮层内输入 |
| `autofocus` | 默认获取焦点 | `boolean` | `false` | - | - |
Copy link

Choose a reason for hiding this comment

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

This code patch looks to be updating the type of the "control" property from string | number | AbstractControl to string | number | (string | number)[] | AbstractControl. This is a valid change and should not cause any issues. However, it is important to make sure that the code is tested properly with different types of inputs for the "control" property, as well as different types of values being passed into it, to ensure that the code works as expected and no unexpected behavior or errors occur. Additionally, it would be good to add a comment near this line of code to explain why the change was made.

Expand Down
5 changes: 4 additions & 1 deletion packages/components/date-picker/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ export interface TimePanelOptions extends PickerTimePanelOptions {
}

const datePickerCommonProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
cellTooltip: Function as PropType<(cell: { value: Date; disabled: boolean }) => string | void>,
open: {
type: Boolean,
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The code patch looks valid and is properly formatted.
  2. The change made to the control prop type looks like a good improvement, as it now accepts an array of string or number values in addition to the existing types.
  3. The cellTooltip prop type could be updated with a more specific type, such as PropType<(cell: { value: Date; disabled: boolean }) => string | void>, as this would provide better type safety.
  4. No other bugs or risks appear to be present.

Expand Down
6 changes: 3 additions & 3 deletions packages/components/form/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `colonless` | 配置 `IxFormItem` 的 `colonless` 默认值 | `boolean` | `false` | ✅ | `seer` 主题默认为 `true` |
| `control` | 表单的控制器 | `string \| number \| AbstractControl` | - | - | 通常是配合 `useFormGroup` 使用 |
| `control` | 表单的控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 通常是配合 `useFormGroup` 使用 |
| `controlCol` | 配置 `IxFormItem` 的 `controlCol` 默认值 | `number \| ColProps` | - | - | - |
| `controlTooltipIcon` | 配置表单控件的提示信息icon | `string` | `'info-circle'` | ✅ | - |
| `labelAlign` | 配置 `IxFormItem` 的 `labelAlign` 默认值 | `'start' \| 'end'` | `'end'` | ✅ | `seer` 主题默认为 `'start'` |
Expand All @@ -28,7 +28,7 @@
| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `colonless` | 是否不显示 `label` 后面的冒号 | `boolean` | - | - | - |
| `control` | 表单控件的控制器 | `string \| number \| AbstractControl` | - | - | 默认取第 1 个子输入控件的 control，如果存在多个输入控件，建议手动指定，参考示例中的 `Phone Number`|
| `control` | 表单控件的控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 默认取第 1 个子输入控件的 control，如果存在多个输入控件，建议手动指定，参考示例中的 `Phone Number`|
| `controlCol` | 配置表单控件的布局配置，可参考 `IxCol` 组件 | `number \| ColProps` | - | - | 传入 `string` 或者 `number` 时，为 `IxCol` 的 `span` 配置 |
| `controlTooltip` | 配置表单控件的提示信息 | `string \| #controlTooltip` | - | - | 通常用于对输入规则的详细说明 |
| `controlTooltipIcon` | 配置表单控件的提示信息icon | `string` | - | - | - |
Expand All @@ -52,7 +52,7 @@

| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `control` | 表单控件的控制器 | `string \| number \| AbstractControl` | - | - | - |
| `control` | 表单控件的控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | - |

### useFormItemRegister

Copy link

Choose a reason for hiding this comment

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

the code review:

  1. In line 6, the type of control has been changed from string \| number \| AbstractControl to string \| number \| (string \| number)[] \| AbstractControl, which allows the user to input an array of strings or numbers. This is useful for scenarios where a form requires multiple input controls from the same user.
  2. In line 28, the same has been done for the control field, allowing for more flexibility and user input.
  3. In line 52, the control field has been changed from string \| number \| AbstractControl to string \| number \| (string \| number)[] \| AbstractControl to allow for the same flexibility as in lines 6 and 28.

Overall, the changes made seem to be beneficial and will add more flexibility to the code. There does not appear to be any bugs or risks associated with the changes.

Expand Down
15 changes: 12 additions & 3 deletions packages/components/form/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ import type { DefineComponent, HTMLAttributes, PropType } from 'vue'
const colProp = [Number, String, Object] as PropType<number | string | ColProps>

export const formProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
colonless: { type: Boolean, default: undefined },
controlCol: colProp,
controlTooltipIcon: String,
Expand All @@ -35,7 +38,10 @@ export type FormInstance = InstanceType<DefineComponent<FormProps>>

export const formItemProps = {
colonless: { type: Boolean, default: undefined },
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
controlCol: colProp,
controlTooltip: String,
controlTooltipIcon: String,
Expand All @@ -62,7 +68,10 @@ export type FormItemComponent = DefineComponent<
export type FormItemInstance = InstanceType<DefineComponent<FormItemProps>>

export const formWrapperProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
} as const

export type FormWrapperProps = ExtractInnerPropTypes<typeof formWrapperProps>
Copy link

Choose a reason for hiding this comment

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

from the beginning.

The code patch basically adds support for Arrays to be passed as a PropType for the control prop in FormProps, FormItemProps and FormWrapperProps.

From a bug-risk perspective, this looks like a safe change as it just adds another type to the list of accepted PropTypes. However, it's important to make sure that the code is tested thoroughly to ensure that there are no unexpected behavior when an array is passed as a PropType.

In terms of improvement suggestions, it could be useful to add some additional documentation to explain the change, so that other developers understand why it was added and how to use it. Additionally, more tests could be added to cover the new functionality.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/input-number/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `v-model:value` | 控件值 | `number` | - | - | 使用 `control` 时，此配置无效 |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `addonAfter` | 设置后缀 | `string \| #addonAfter` | - | - | - |
| `addonBefore` | 设置前缀 | `string \| #addonBefore` | - | - | - |
| `disabled` | 禁用 | `boolean` | `false` | - | 使用 `control` 时，此配置无效 |
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The patch seems to be updating the type of the control property from string, number or AbstractControl to string, number, (string or number)[] or AbstractControl. This change should be properly tested to make sure that no errors are encountered during runtime.

  2. It is also important to check if the new type is valid and compatible with the existing code. If not, then appropriate changes must be made to make it compatible with the existing code.

  3. In addition, it is necessary to ensure that all the changes made by the patch do not break any existing functionality. Any unintended side effects should be identified and fixed.

  4. Lastly, it is important to ensure that no security risks are introduced by the patch. Appropriate measures must be taken to ensure the patch does not introduce any vulnerabilities.

Expand Down
5 changes: 4 additions & 1 deletion packages/components/input-number/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ export type InputNumberButtonPosition = 'inner' | 'outer'

export const inputNumberProps = {
value: [Number, null] as PropType<number | null>,
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
disabled: {
type: Boolean,
default: false,
Copy link

Choose a reason for hiding this comment

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

with the code review:

  1. The code patch provided is valid and does not contain any syntax errors.
  2. The change made in the code patch is to change the type of the 'control' prop to accept an array of strings or numbers as well as an AbstractControl. This is a valid change and adds flexibility.
  3. The code also contains proper type declarations and is properly formatted, so there are no changes needed from that perspective.
  4. The only improvement suggestion would be to add more comments and documentation to explain why this change was made and how it works. This will help other developers understand the code more easily.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/input/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `v-model:value` | 控件值 | `string` | - | - | 使用 `control` 时，此配置无效 |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `addonAfter` | 设置后置标签 | `string \| #addonAfter` | - | - | - |
| `addonBefore` | 设置前置标签 | `string \| #addonBefore` | - | - | - |
| `borderless` | 是否显示边框 | `boolean` | `false` | ✅ | - |
Copy link

Choose a reason for hiding this comment

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

:

The code patch looks to add a type to the control option. The new type is (string | number)[] in addition to what was already existing: string | number | AbstractControl.

This change looks valid and should not introduce any bugs, however it is important to test the change thoroughly to make sure everything works as expected.

In terms of improvement suggestions, it would be helpful to provide an example of how to use the new type of control option. This will help other users understand how to use this option more easily.

Expand Down
8 changes: 4 additions & 4 deletions packages/components/input/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ import type { ExtractInnerPropTypes, ExtractPublicPropTypes, MaybeArray } from '
import type { FormSize } from '@idux/components/form'
import type { DefineComponent, InputHTMLAttributes, PropType } from 'vue'

export type TextareaResize = 'none' | 'both' | 'horizontal' | 'vertical'
export type TextareaAutoRows = { minRows: number; maxRows: number }

export const inputCommonProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
value: { type: String, default: undefined },

clearable: { type: Boolean, default: undefined },
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The first thing to note is that the patch changes the type of the control prop from String, Number, Object to String, Number, Object, Array. This could potentially cause issues since it is now possible for a user to provide an array as the value for the control prop. To prevent any potential issues, it is recommended to add more validation checks to make sure only valid values are accepted.

  2. It is also recommended to add comments to the code to explain why this change was made and how it affects the functionality of the code. This can help future developers understand why this change was made and how it affects the code.

  3. Finally, it is also recommended to add tests to the code to make sure that this change works as expected and that it does not introduce any bugs. This will help ensure that the code functions correctly and that any potential issues are caught before they become a problem.

Expand Down
4 changes: 2 additions & 2 deletions packages/components/radio/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `v-model:checked` | 是否选中 | `boolean` | - | - | 使用 `control` 时，此配置无效 |
| `autofocus` | 是否以自动聚焦 | `boolean` | `false` | - | - |
| `buttoned` | 是否以按钮显示 | `boolean` | `false` | - | - |
Expand Down Expand Up @@ -34,7 +34,7 @@

| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `v-model:value` | 当前选中的值 | `any` | - | - | 使用 `control` 时，此配置无效 |
| `buttoned` | 设置单选框组内 `IxRadio` 的 `buttoned` | `boolean` | - | - | - |
| `dataSource` | 以配置形式设置子元素 | `RadioData[]`| - | 优先级高于 `default` 插槽 | |
Copy link

Choose a reason for hiding this comment

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

the code review:

First, the code patch looks good. It is adding an additional type option to the control parameter of the IxRadio and IxRadioGroup components. This should not introduce any bugs and should be safe to use.

Some potential improvements that could be made are:

  1. Adding more documentation and comments to explain the purpose of the new parameter.
  2. Doing some testing to make sure the new parameter works as expected.
  3. Adding an example of how to use the new parameter in the documentation.

Expand Down
10 changes: 8 additions & 2 deletions packages/components/radio/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ import type { SpaceProps } from '@idux/components/space'
import type { DefineComponent, HTMLAttributes, LabelHTMLAttributes, PropType } from 'vue'

export const radioProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
checked: { type: Boolean, default: undefined },

autofocus: { type: Boolean, default: false },
Expand Down Expand Up @@ -46,7 +49,10 @@ export type RadioComponent = DefineComponent<
export type RadioInstance = InstanceType<DefineComponent<RadioProps, RadioBindings>>

export const radioGroupProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
value: { type: null, default: undefined },

buttoned: { type: Boolean, default: false },
Copy link

Choose a reason for hiding this comment

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

with a brief code review.

The first thing I noticed is that you have added a new type to the radioProps control prop - Array. This will allow you to accept arrays as values for the control prop. This could potentially introduce some bugs into the code if not handled correctly but it also opens up a lot of possibilities for the code.

The second change is to the radioGroupProps control prop. This also has the Array type added to it. This allows for an array to be passed as the value for this prop. Again, this could introduce some bugs if not handled correctly.

Overall, these changes appear to make the code more flexible and open up new possibilities. However, it is important to ensure that the code is tested thoroughly to make sure that the new types are handled correctly and that any potential bugs are caught before they cause errors in production.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/rate/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `v-model:value` | 高亮的数目（分数） | `number` | - | - | 使用 `control` 时，此配置无效 |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `allowHalf` | 支持半分（选） | `boolean` | `false` | ✅ | - |
| `clearable` | 二次点击元素后清除选择 | `boolean` | `false` | ✅ |-|
| `count` | 图标数目 | `number` | `5` | ✅ | - |
Copy link

Choose a reason for hiding this comment

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

the review:

  1. The patch adds the ability to accept an array of values for the control field. This adds flexibility for users to customize the behavior and allows for more use cases.
  2. The patch adds the ability to support half scores, which can be useful in some scenarios.
  3. The patch adds clearable functionality, which allows users to quickly reset the selected values when needed.
  4. The patch also adds a count field, which allows users to control the number of icons displayed.

Overall, this patch looks good and adds useful features to the code. However, it would be beneficial to add unit tests to ensure that the added features are working as expected. Additionally, it would be worth considering adding more documentation to explain the new features.

Expand Down
5 changes: 4 additions & 1 deletion packages/components/rate/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import type { FormSize } from '@idux/components/form'
import type { DefineComponent, HTMLAttributes, PropType } from 'vue'

export const rateProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
value: [Number, String] as PropType<number | string>,

allowHalf: {
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The code patch is valid, it adds an additional type to the "control" prop in the "rateProps" object, which allows it to also accept arrays with strings and numbers.
  2. The code is efficient and well organized and there are no obvious bugs.
  3. As a suggestion, it might be worth adding a comment that explains why the new type was added. This will make future maintenance easier.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/select/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `v-model:value` | 当前选中的 option 的值 | `any \| any[]` | - | - | 当 `multiple=true` 时，`value` 为数组，使用 `control` 时，此配置无效 |
| `v-model:open` | 下拉菜单是否展开 | `boolean` | - | - | - |
| `allowInput` | 允许输入模式 | `boolean` | `false` | - | - |
Copy link

Choose a reason for hiding this comment

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

the code review

  1. The code patch is syntactically correct and looks to be properly formatted.
  2. The patch is applying a type change in the control property from string \| number \| AbstractControl to string \| number \| (string \| number)[] \| AbstractControl which looks appropriate.
  3. There is no bug risk identified in the patch.
  4. There is no improvement suggestion identified in the patch.

Expand Down
5 changes: 4 additions & 1 deletion packages/components/select/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ export const selectPanelProps = {
} as const

export const selectProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
value: { type: [String, Number, Symbol, Array] as PropType<MaybeArray<VKey>>, default: undefined },
open: { type: Boolean, default: undefined },

Copy link

Choose a reason for hiding this comment

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

the code review

The code looks pretty good. The only thing that stands out is the change made on line 46, where the PropType was changed from [String, Number, Object] to [String, Number, Object, Array]. It looks like this was done to allow for an array of strings or numbers, but there is a risk that it could cause unexpected behavior if an array of objects is passed in. I would recommend adding some console logging to check that the type of control being passed in is valid before allowing it to be passed in. This can help you prevent any unexpected bugs.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/slider/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `v-model:value` | 绑定值 | `number \| [number, number]` | `0 \| [0, 0]` | - | 使用 `control` 时，此配置无效 |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `disabled` | 设置禁用状态 | `boolean` | `false` | - | 使用 `control` 时，此配置无效 |
| `dots` | 显示间断点 | `boolean` | `false` | - | `marks` 间断点会始终显示 |
|`marks`|刻度标记，`key` 的类型必须为 `number` 且取值在闭区间 `[min, max]` 内，每个标签可以单独设置样式|`object`|-|-|`{ number: string \| VNode } or { number: { style: object, label: string \| VNode } } or { number: () => VNode }` |
Copy link

Choose a reason for hiding this comment

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

with a code review.

The patch is a change to the configuration options of a slider component, adding a new type to the "control" option. It appears that the control option which previously accepted strings, numbers and AbstractControls, now also accepts arrays of strings or numbers. The type of the key for the marks option has also been changed from string to number, and the type of each label for the marks option has been changed from string to string or VNode.

Overall, this code looks valid and should not introduce any bugs. However, it would be beneficial to provide a more detailed explanation in the comments about how the changes will affect the slider component. Additionally, it would be helpful to provide some examples for how to use the new features.

Expand Down
5 changes: 4 additions & 1 deletion packages/components/slider/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import type { CSSProperties, DefineComponent, HTMLAttributes, PropType, VNode }

// slider
export const sliderProps = {
control: { type: [String, Number, Object] as PropType<string | number | AbstractControl>, default: undefined },
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
value: { type: [Number, Array] as PropType<number | number[]>, default: undefined },

disabled: { type: Boolean, default: false },
Copy link

Choose a reason for hiding this comment

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

with a code review

First of all, the code looks well written with good formatting and indentation. It is also following the best practices of setting up the props with the PropType<> generic to ensure type safety.

The only issue I can spot is that in the control prop, instead of just setting [String, Number, Object] as the type, it should also include Array since it accepts an array of strings and numbers too. So the code should be updated to:

control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},

Other than that, everything seems okay and there are no other risks or improvement suggestions that come to mind.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/switch/docs/Api.zh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 |
| --- | --- | --- | --- | --- | --- |
| `control` | 控件控制器 | `string \| number \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `control` | 控件控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | 配合 `@idux/cdk/forms` 使用, 参考 [Form](/components/form/zh) |
| `v-model:checked` | 是否开启 | `boolean` | `false` | - | 使用 `control` 时，此配置无效 |
| `autofocus` | 自动获取焦点 | `boolean` | `false` | - | - |
| `disabled` | 是否禁止操作 | `boolean` | `false`| - | 使用 `control` 时，此配置无效 |
Copy link

Choose a reason for hiding this comment

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

the review:

  1. The code patch looks good and seems to be working properly.
  2. There are no obvious bugs that could cause any major issues.
  3. The addition of the (string \| number)[] type to the control property is a good improvement, as it allows for more flexible control over the input.
  4. The comments have been update to provide more clarity on how the control property should be used.
  5. The autofocus and disabled properties are set to false by default, which is a good practice.

Expand Down
6 changes: 5 additions & 1 deletion packages/components/switch/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@ import type { ExtractInnerPropTypes, ExtractPublicPropTypes, MaybeArray } from '
import type { DefineComponent, HTMLAttributes, PropType } from 'vue'

export const switchProps = {
control: {
type: [String, Number, Object, Array] as PropType<string | number | (string | number)[] | AbstractControl>,
default: undefined,
},
checked: { type: Boolean, default: undefined },
control: [String, Number, Object] as PropType<string | number | AbstractControl>,

autofocus: { type: Boolean, default: false },
disabled: { type: Boolean, default: false },
labels: { type: Array as PropType<string[]>, default: () => [] },
Copy link

Choose a reason for hiding this comment

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

with a brief code review.

First, the code is syntactically correct and does not have any syntax errors. The changes made to the switchProps object appear to be valid, as the control property has been updated to accept an array of strings and numbers in addition to a string or number value. Additionally, the default value for the control property has been set to 'undefined'.

In terms of bugs, there currently doesn't appear to be any that can be identified from the code snippet provided. However, it is important to note that this code should be tested thoroughly for proper functionality if it is to be used in production.

In terms of improvements, the code could be improved by adding comments to explain the purpose of each line of code. Additionally, the code could be refactored to use more descriptive variable names and be split into smaller functions to make it easier to read and debug.

Expand Down
Loading