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

修改picker组件issue #2360

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

修改picker组件issue #2360

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7116dd
fix: 解决Picker 和 FormItem 搭配使用时，ref.open() 会触发 Picker 的 onClose问题 --is…
Jun 18, 2024
6aba177
fix: 修改onChange触发时机，解决第一次打开就触发onChange且传值错误的问题
Jun 18, 2024
76a5e85
fix: 修改form下的picker在第一次不选择选项，直接取消选择后再次点击picker，出现第一项被选中的问题 --issue#2290
Jun 18, 2024
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
16 changes: 9 additions & 7 deletions src/packages/picker/picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ const InternalPicker: ForwardRefRenderFunction<
defaultValue: [...defaultValue],
finalValue: [...defaultValue],
onChange: (val: (string | number)[]) => {
props.onConfirm?.(setSelectedOptions(), val)
!val && props.onConfirm?.(setSelectedOptions(), val)
Copy link

Choose a reason for hiding this comment

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

代码逻辑存在潜在问题。当 val 为 undefined 或 null 时，onConfirm 事件仍会被调用，这可能不是预期行为。

- !val && props.onConfirm?.(setSelectedOptions(), val)
+ val && props.onConfirm?.(setSelectedOptions(), val)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
!val && props.onConfirm?.(setSelectedOptions(), val)
val && props.onConfirm?.(setSelectedOptions(), val)

},
})
const [innerVisible, setInnerVisible] = usePropsValue<boolean>({
Expand Down Expand Up @@ -197,7 +197,11 @@ const InternalPicker: ForwardRefRenderFunction<
}

useEffect(() => {
setInnerValue(innerValue !== selectedValue ? selectedValue : innerValue)
// 此hook的作用是‘如果内部选中值与用户选中值不同则把内部值置用户选中值’保证用户打开选项时选中的是选择的值。
// 但是当用户并没有进行确认选择，则不需要进行修改innerValue，否则会出现 issue#2290的问题
if (innerValue !== selectedValue && selectedValue.length > 0) {
setInnerValue(selectedValue)
}
Comment on lines +200 to +204
Copy link

Choose a reason for hiding this comment

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

此处的逻辑处理可能会导致不必要的状态更新，建议仅在 selectedValue 有实质性变化时更新 innerValue。

- if (innerValue !== selectedValue && selectedValue.length > 0) {
+ if (!isEqual(innerValue, selectedValue) && selectedValue.length > 0) {
    setInnerValue(selectedValue)
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 此hook的作用是‘如果内部选中值与用户选中值不同则把内部值置用户选中值’保证用户打开选项时选中的是选择的值。
// 但是当用户并没有进行确认选择，则不需要进行修改innerValue，否则会出现 issue#2290的问题
if (innerValue !== selectedValue && selectedValue.length > 0) {
setInnerValue(selectedValue)
}
// 此hook的作用是‘如果内部选中值与用户选中值不同则把内部值置用户选中值’保证用户打开选项时选中的是选择的值。
// 但是当用户并没有进行确认选择，则不需要进行修改innerValue，否则会出现 issue#2290的问题
if (!isEqual(innerValue, selectedValue) && selectedValue.length > 0) {
setInnerValue(selectedValue)
}

}, [innerVisible, selectedValue])

useEffect(() => {
Expand All @@ -206,11 +210,6 @@ const InternalPicker: ForwardRefRenderFunction<
}
}, [options, innerVisible])

// 选中值进行修改
useEffect(() => {
onChange && onChange(setSelectedOptions(), innerValue, columnIndex)
}, [innerValue, columnsList])

const setSelectedOptions = () => {
const options: PickerOption[] = []
let currOptions = []
Expand Down Expand Up @@ -251,6 +250,8 @@ const InternalPicker: ForwardRefRenderFunction<
]
setInnerValue(combineResult)
setColumnsList(normalListData(combineResult) as PickerOption[][])

onChange && onChange(setSelectedOptions(), combineResult, columnIndex)
Comment on lines +253 to +254
Copy link

Choose a reason for hiding this comment

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

建议使用可选链操作符来简化代码。

- onChange && onChange(setSelectedOptions(), combineResult, columnIndex)
+ onChange?.(setSelectedOptions(), combineResult, columnIndex)

Also applies to: 262-264

} else {
setInnerValue((data: (number | string)[]) => {
const cdata: (number | string)[] = [...data]
Expand All @@ -260,6 +261,7 @@ const InternalPicker: ForwardRefRenderFunction<
)
? columnOptions.value
: ''
onChange && onChange(setSelectedOptions(), cdata, columnIndex)
return cdata
})
}
Expand Down