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

enhance: Lock switch field if needConfirm #892

Merged
merged 3 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
41 changes: 22 additions & 19 deletions docs/examples/debug.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,32 +55,35 @@ export default () => {
{...sharedLocale}
style={{ width: 400 }}
showTime
// allowEmpty
// disabledDate={(_, info) => {
// console.log('Date:', info);
// return false;
// }}
disabledTime={(date, range, info) => {
// console.log(`Time-${range}`, range, info);
const { from } = info;
// disabledTime={(date, range, info) => {
// // console.log(`Time-${range}`, range, info);
// const { from } = info;

if (from) {
console.log(
`Time-${range}`,
from.format('YYYY-MM-DD HH:mm:ss'),
date.format('YYYY-MM-DD HH:mm:ss'),
);
}
// if (from) {
// console.log(
// `Time-${range}`,
// from.format('YYYY-MM-DD HH:mm:ss'),
// date.format('YYYY-MM-DD HH:mm:ss'),
// );
// }

if (from && from.isSame(date, 'day')) {
return {
disabledHours: () => [from.hour()],
disabledMinutes: () => [0, 1, 2, 3],
disabledSeconds: () => [0, 1, 2, 3],
};
}
return {};
}}
// if (from && from.isSame(date, 'day')) {
// return {
// disabledHours: () => [from.hour()],
// disabledMinutes: () => [0, 1, 2, 3],
// disabledSeconds: () => [0, 1, 2, 3],
// };
// }
// return {};
// }}
Comment on lines +63 to +83
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议删除注释代码而不是保留

建议完全删除这段被注释的代码,而不是将其保留在文件中。如果这些代码对于示例或测试很重要,应该:

  1. 将其移动到单独的示例文件中
  2. 或者在文档中说明这种用法

保留注释掉的代码会:

  • 增加维护负担
  • 造成代码混乱
  • 可能导致过时的实现被重新使用

/>

<RangePicker {...sharedLocale} style={{ width: 400 }} allowEmpty />
{/* <SinglePicker
{...dateFnsSharedLocale}
style={{ width: 400 }}
Expand Down
20 changes: 19 additions & 1 deletion src/PickerInput/RangePicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import useRangeDisabledDate from './hooks/useRangeDisabledDate';
import useRangePickerValue from './hooks/useRangePickerValue';
import useRangeValue, { useInnerValue } from './hooks/useRangeValue';
import useShowNow from './hooks/useShowNow';
import Popup, { PopupShowTimeConfig } from './Popup';
import Popup, { type PopupShowTimeConfig } from './Popup';
import RangeSelector, { type SelectorIdType } from './Selector/RangeSelector';

function separateConfig<T>(config: T | [T, T] | null | undefined, defaultConfig: T): [T, T] {
Expand Down Expand Up @@ -325,6 +325,8 @@ function RangePicker<DateType extends object = any>(
flushSubmit,
/** Trigger `onChange` directly without check `disabledDate` */
triggerSubmitChange,
/** Tell `index` has filled value in it */
hasSubmitValue,
] = useRangeValue<RangeValueType<DateType>, DateType>(
filledProps,
mergedValue,
Expand Down Expand Up @@ -630,6 +632,22 @@ function RangePicker<DateType extends object = any>(

// ======================= Selector =======================
const onSelectorFocus: SelectorProps['onFocus'] = (event, index) => {
// Check if `needConfirm` but user not submit yet
const activeListLen = activeIndexList.length;
const lastActiveIndex = activeIndexList[activeListLen - 1];
if (
activeListLen &&
lastActiveIndex !== index &&
needConfirm &&
// Not change index if is not filled
!allowEmpty[lastActiveIndex] &&
!hasSubmitValue(lastActiveIndex) &&
calendarValue[lastActiveIndex]
) {
selectorRef.current.focus({ index: lastActiveIndex });
return;
}

lastOperation('input');

triggerOpen(true, {
Expand Down
23 changes: 23 additions & 0 deletions src/PickerInput/hooks/useRangeActiveLock.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import * as React from 'react';

export default function useRangeActiveLock(): [
focused: boolean,
triggerFocus: (focused: boolean) => void,
// lastOperation: (type?: OperationType) => OperationType,
// activeIndex: number,
setActiveIndex: (index: number) => void,
// nextActiveIndex: NextActive<DateType>,
// activeList: number[],
] {
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

类型定义与实现不匹配

返回类型签名中包含了被注释掉的类型定义,这与实际实现的返回值不一致。建议移除未使用的类型定义或实现完整功能。

建议应用以下更改:

export default function useRangeActiveLock(): [
  focused: boolean,
  triggerFocus: (focused: boolean) => void,
-  // lastOperation: (type?: OperationType) => void,
-  // activeIndex: number,
  setActiveIndex: (index: number) => void,
-  // nextActiveIndex: NextActive<DateType>,
-  // activeList: number[],
] {
📝 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
export default function useRangeActiveLock(): [
focused: boolean,
triggerFocus: (focused: boolean) => void,
// lastOperation: (type?: OperationType) => OperationType,
// activeIndex: number,
setActiveIndex: (index: number) => void,
// nextActiveIndex: NextActive<DateType>,
// activeList: number[],
] {
export default function useRangeActiveLock(): [
focused: boolean,
triggerFocus: (focused: boolean) => void,
setActiveIndex: (index: number) => void,
] {

const [activeIndex, setActiveIndex] = React.useState<number | null>(null);
const [focused, setFocused] = React.useState<boolean>(false);
const [activeList, setActiveList] = React.useState<number[]>([]);

// ============================= Active =============================
const onActive = (index: number) => {
setActiveIndex(index);
setActiveList([...activeList, index]);
};
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

潜在的内存泄漏问题

activeList 数组只会不断增长而没有清理机制,可能导致内存泄漏。建议限制数组大小或在适当时机清理。

建议修改实现方式:

const onActive = (index: number) => {
  setActiveIndex(index);
-  setActiveList([...activeList, index]);
+  setActiveList(prevList => {
+    const newList = [...prevList, index];
+    // 只保留最近的 N 个记录
+    return newList.slice(-5);
+  });
};
📝 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
const onActive = (index: number) => {
setActiveIndex(index);
setActiveList([...activeList, index]);
};
const onActive = (index: number) => {
setActiveIndex(index);
setActiveList(prevList => {
const newList = [...prevList, index];
// 只保留最近的 N 个记录
return newList.slice(-5);
});
};


return [focused, setFocused, onActive];
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

返回值命名不一致

返回值中 setFocused 与类型定义中的 triggerFocus 命名不一致,onActivesetActiveIndex 命名不一致。

建议修改为:

- return [focused, setFocused, onActive];
+ return [focused, triggerFocus: setFocused, setActiveIndex: onActive];

Committable suggestion skipped: line range outside the PR's diff.

}
11 changes: 9 additions & 2 deletions src/PickerInput/hooks/useRangeValue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ function orderDates<DateType extends object, DatesType extends DateType[]>(
* Used for internal value management.
* It should always use `mergedValue` in render logic
*/
export function useCalendarValue<MergedValueType extends object[]>(mergedValue: MergedValueType) {
function useCalendarValue<MergedValueType extends object[]>(mergedValue: MergedValueType) {
const [calendarValue, setCalendarValue] = useSyncState(mergedValue);

/** Sync calendarValue & submitValue back with value */
Expand Down Expand Up @@ -186,6 +186,8 @@ export default function useRangeValue<ValueType extends DateType[], DateType ext
flushSubmit: (index: number, needTriggerChange: boolean) => void,
/** Trigger `onChange` directly without check `disabledDate` */
triggerSubmitChange: (value: ValueType) => boolean,
/** Tell `index` has filled value in it */
hasSubmitValue: (index: number) => boolean,
] {
const {
// MISC
Expand Down Expand Up @@ -331,6 +333,11 @@ export default function useRangeValue<ValueType extends DateType[], DateType ext
2,
);

// ============================ Check =============================
function hasSubmitValue(index: number) {
return !!submitValue()[index];
}

// ============================ Return ============================
return [flushSubmit, triggerSubmit];
return [flushSubmit, triggerSubmit, hasSubmitValue];
}
17 changes: 17 additions & 0 deletions tests/range.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ describe('Picker.Range', () => {
resetWarned();
global.scrollCalled = false;
jest.useFakeTimers().setSystemTime(getDay('1990-09-03 00:00:00').valueOf());
document.body.innerHTML = '';
});

afterEach(() => {
Expand Down Expand Up @@ -2054,4 +2055,20 @@ describe('Picker.Range', () => {
'rc-picker-input-active',
);
});

it('should not click to focus on next field if first field is not confirm', () => {
const onCalendarChange = jest.fn();
const { container } = render(
<DayRangePicker onCalendarChange={onCalendarChange} showTime needConfirm />,
);

// Select first field
openPicker(container, 0);
selectCell(11);
expect(onCalendarChange).toHaveBeenCalled();

// Not click confirm and click next field
openPicker(container, 1);
expect(container.querySelectorAll('.rc-picker-input')[0]).toHaveClass('rc-picker-input-active');
});
});
Loading