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

fix: interacting with footer components closes the date selection panel #683

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions src/Picker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ function InnerPicker<DateType>(props: PickerProps<DateType>) {
autoComplete = 'off',
inputRender,
changeOnBlur,
renderExtraFooter,
} = props as MergedPickerProps<DateType>;

const inputRef = React.useRef<HTMLInputElement>(null);
Expand Down Expand Up @@ -326,6 +327,7 @@ function InnerPicker<DateType>(props: PickerProps<DateType>) {
const [inputProps, { focused, typing }] = usePickerInput({
blurToCancel: needConfirmButton,
changeOnBlur,
hasExtraFooter: !!renderExtraFooter,
Copy link
Contributor

Choose a reason for hiding this comment

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

usePickerInput 可以直接接收 renderExtraFooter 吗,不需要用 hasExtraFooter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

里面主要用来做逻辑判断应该用不到 renderExtraFooter 方法,因此我在外面判断是否存在。

open: mergedOpen,
value: text,
triggerOpen,
Expand Down
1 change: 1 addition & 0 deletions src/RangePicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ function InnerRangePicker<DateType>(props: RangePickerProps<DateType>) {
const getSharedInputHookProps = (index: 0 | 1, resetText: () => void) => ({
blurToCancel: !changeOnBlur && needConfirmButton,
changeOnBlur,
hasExtraFooter: !!renderExtraFooter,
forwardKeyDown,
onBlur: onInternalBlur,
isClickOutside: (target: EventTarget | null) => {
Expand Down
12 changes: 10 additions & 2 deletions src/hooks/usePickerInput.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export default function usePickerInput({
onKeyDown,
blurToCancel,
changeOnBlur,
hasExtraFooter,
onSubmit,
onCancel,
onFocus,
Expand All @@ -25,14 +26,16 @@ export default function usePickerInput({
forwardKeyDown: (e: React.KeyboardEvent<HTMLInputElement>) => boolean;
onKeyDown: (e: React.KeyboardEvent<HTMLInputElement>, preventDefault: () => void) => void;
blurToCancel?: boolean;
changeOnBlur?: boolean
changeOnBlur?: boolean;
hasExtraFooter?: boolean;
onSubmit: () => void | boolean;
onCancel: () => void;
onFocus?: React.FocusEventHandler<HTMLInputElement>;
onBlur?: React.FocusEventHandler<HTMLInputElement>;
}): [React.DOMAttributes<HTMLInputElement>, { focused: boolean; typing: boolean }] {
const [typing, setTyping] = useState(false);
const [focused, setFocused] = useState(false);
const [outside, setOutside] = useState(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

这个改变需要 render 吗,可以用 useRef 吗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

对,只需要阻塞一下 blur。


/**
* We will prevent blur to handle open event when user click outside,
Expand Down Expand Up @@ -108,7 +111,7 @@ export default function usePickerInput({
},

onBlur: (e) => {
if (preventBlurRef.current || !isClickOutside(document.activeElement)) {
if (preventBlurRef.current || !isClickOutside(document.activeElement) || !outside) {
preventBlurRef.current = false;
return;
}
Expand Down Expand Up @@ -152,6 +155,11 @@ export default function usePickerInput({
const target = getTargetFromEvent(e);
const clickedOutside = isClickOutside(target);

if (hasExtraFooter) {
Copy link
Member

Choose a reason for hiding this comment

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

这种零碎逻辑太多了不好,感觉是写个 RFC 让 ConfigProvider 支持关闭逻辑。例如默认是 blur 关闭,也可以设置成点击外部关闭。这样就不管你是哪个组件,都是一样的逻辑。

Copy link
Contributor

Choose a reason for hiding this comment

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

image

footer 可以是按钮,需要点击后关闭 Picker,如果是 Input 就不需要关闭,最终还是要在 Picker 支持 props 控制是否可以关闭吧?

Copy link
Contributor

@crazyair crazyair Oct 10, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

footer 可以是按钮,需要点击后关闭 Picker,如果是 Input 就不需要关闭,最终还是要在 Picker 支持 props 控制是否可以关闭吧?

嗯嗯

Copy link
Contributor

Choose a reason for hiding this comment

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

另外,点击 renderExtraFooter 位置元素,应该不属于 blur 啊?也没有离开 Picker 啊?为什么会触发 blur 呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

触发了 input 的 blur 。

Copy link
Contributor

Choose a reason for hiding this comment

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

还有 Select 的自定义 render,也需要阻止冒泡。按理说点击自定义添加的按钮或者 Input,应该不属于 Select 的选中,不应该关闭 Select 的下拉框的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

对的,所以这里可以整理一个 RFC 出来。

// Save whether the last click is inside the component when `extraFooter` exists.
setOutside(clickedOutside);
}

if (open) {
if (!clickedOutside) {
preventBlurRef.current = true;
Expand Down
15 changes: 15 additions & 0 deletions tests/picker.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1145,4 +1145,19 @@ describe('Picker.Basic', () => {

expect(container.querySelector('input')).toHaveValue('2023-09-04 21:05:10');
});

it('interacting with components within footer should not close the panel', () => {
const { container, baseElement } = render(
<MomentPicker renderExtraFooter={() => <button className="test-button">button</button>} />,
);

openPicker(container);

fireEvent.click(baseElement.querySelector('.test-button'));

// Simulate component behavior
fireEvent.blur(container.querySelector('input'));

expect(baseElement.querySelector('.rc-picker-dropdown-hidden')).toBeFalsy();
});
});
18 changes: 18 additions & 0 deletions tests/range.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1985,4 +1985,22 @@ describe('Picker.Range', () => {

expect(onOpenChange).toHaveBeenCalledWith(false);
});

// In line with the picker
it('interacting with components within footer should not close the panel', () => {
const { container, baseElement } = render(
<MomentRangePicker
renderExtraFooter={() => <button className="test-button">button</button>}
/>,
);

openPicker(container);

fireEvent.click(baseElement.querySelector('.test-button'));

// Simulate component behavior
fireEvent.blur(container.querySelector('input'));

expect(baseElement.querySelector('.rc-picker-dropdown-hidden')).toBeFalsy();
});
});
Loading