-
Notifications
You must be signed in to change notification settings - Fork 138
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(comp:*): the control type of all form components is incomplete #1495
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` | - | - | - | | ||
|
@@ -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` 插槽 | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }, | ||
|
@@ -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 }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the review.
Overall, the code looks good. Adding validation and making use of the unused props would improve it further. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` | - | - | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code review:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'` | | ||
|
@@ -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` | - | - | - | | ||
|
@@ -52,7 +52,7 @@ | |
|
||
| 名称 | 说明 | 类型 | 默认值 | 全局配置 | 备注 | | ||
| --- | --- | --- | --- | --- | --- | | ||
| `control` | 表单控件的控制器 | `string \| number \| AbstractControl` | - | - | - | | ||
| `control` | 表单控件的控制器 | `string \| number \| (string \| number)[] \| AbstractControl` | - | - | - | | ||
|
||
### useFormItemRegister | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code review:
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. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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, | ||
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` 时,此配置无效 | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code review:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with the code review:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` | ✅ | - | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. : The code patch looks to add a type to the 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 |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code review:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` | - | - | | ||
|
@@ -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` 插槽 | | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }, | ||
|
@@ -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 }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` | ✅ | - | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the review:
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. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code review:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` | - | - | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code review
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }, | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }` | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: { Other than that, everything seems okay and there are no other risks or improvement suggestions that come to mind. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` 时,此配置无效 | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the review:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: () => [] }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review
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.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.v-model:expandedKeys
, the type changed from VKey[] to VKey[], which does not change the code's behavior.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.