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

feat: improve focus behavior #681

Merged
merged 5 commits into from
Nov 1, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
22 changes: 22 additions & 0 deletions docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,25 @@ nav:
</tr>
</tbody>
</table>

## inputRef

```tsx | pure
import InputNumber, { InputNumberRef } from 'rc-input-number';

const inputRef = useRef<InputNumberRef>(null);

useEffect(() => {
inputRef.current.focus(); // the input will get focus
inputRef.current.blur(); // the input will lose focus
console.log(inputRef.current.input); // The origin input element
}, []);
// ....
<InputNumber ref={inputRef} />;
```

| Property | Type | Description |
| -------- | --------------------------------------- | --------------------------------- |
| focus | `(options?: InputFocusOptions) => void` | The input get focus when called |
| blur | `() => void` | The input loses focus when called |
| input | `HTMLInputElement \| null` | The origin input element |
28 changes: 28 additions & 0 deletions docs/demo/focus.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/* eslint no-console:0 */
import InputNumber, { InputNumberRef } from 'rc-input-number';
import React from 'react';
import '../../assets/index.less';

export default () => {
const inputRef = React.useRef<InputNumberRef>(null);

return (
<div style={{ margin: 10 }}>
<InputNumber aria-label="focus example" value={10} style={{ width: 100 }} ref={inputRef} />
<div style={{ marginTop: 10 }}>
<button type="button" onClick={() => inputRef.current?.focus({ cursor: 'start' })}>
focus at start
</button>
<button type="button" onClick={() => inputRef.current?.focus({ cursor: 'end' })}>
focus at end
</button>
<button type="button" onClick={() => inputRef.current?.focus({ cursor: 'all' })}>
focus to select all
</button>
<button type="button" onClick={() => inputRef.current?.focus({ preventScroll: true })}>
focus prevent scroll
</button>
</div>
Comment on lines +12 to +25
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. 添加适当的间距
  3. 提供更好的视觉反馈
-<div style={{ marginTop: 10 }}>
+<div
+  style={{
+    marginTop: 10,
+    display: 'flex',
+    gap: 8,
+    flexWrap: 'wrap'
+  }}
+  role="group"
+  aria-label="焦点控制按钮组"
+>
   <button
     type="button"
+    style={{
+      padding: '4px 8px',
+      borderRadius: 4,
+      border: '1px solid #d9d9d9'
+    }}
     onClick={() => inputRef.current?.focus({ cursor: 'start' })}
   >
     focus at start
   </button>
   // 对其他按钮应用相同的样式...
 </div>

Committable suggestion was skipped due to low confidence.

</div>
);
};
2 changes: 2 additions & 0 deletions docs/example.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,6 @@ nav:

<code src="./demo/wheel.tsx"></code>

## focus

<code src="./demo/focus.tsx"></code>
24 changes: 24 additions & 0 deletions src/InputNumber.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ import useFrame from './hooks/useFrame';
export type { ValueType };

export interface InputNumberRef extends HTMLInputElement {
focus: (options?: InputFocusOptions) => void;
blur: () => void;
setSelectionRange: (
aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved
start: number,
end: number,
direction?: 'forward' | 'backward' | 'none',
) => void;
select: () => void;
input: HTMLInputElement | null;
nativeElement: HTMLElement;
}

Expand Down Expand Up @@ -660,6 +669,21 @@ const InputNumber = React.forwardRef<InputNumberRef, InputNumberProps>((props, r

React.useImperativeHandle(ref, () =>
proxyObject(inputFocusRef.current, {
focus,
blur: () => {
Copy link
Member

Choose a reason for hiding this comment

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

proxyObject 本来就支持 blur 的,这边不需要额外处理哈~

inputFocusRef.current?.blur();
},
setSelectionRange: (
Copy link
Member

Choose a reason for hiding this comment

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

这边你忘记删了哈~

Copy link
Member

Choose a reason for hiding this comment

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

从这往下都是不需要的

start: number,
end: number,
direction?: 'forward' | 'backward' | 'none',
) => {
inputFocusRef.current?.setSelectionRange(start, end, direction);
},
select: () => {
inputFocusRef.current?.select();
},
input: inputFocusRef.current,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议优化 focus 和 blur 方法的空值检查

根据之前的讨论,建议仅为 focus 和 blur 方法添加空值检查,以提高代码的健壮性。其他方法可以让用户通过 nativeElement 自行处理。

建议按如下方式修改:

-      focus,
+      focus: (option?: InputFocusOptions) => {
+        if (!inputFocusRef.current) return;
+        triggerFocus(inputFocusRef.current, option);
+      },
       blur: () => {
+        if (!inputFocusRef.current) return;
         inputFocusRef.current?.blur();
       },
       setSelectionRange: (
         start: number,
         end: number,
         direction?: 'forward' | 'backward' | 'none',
       ) => {
         inputFocusRef.current?.setSelectionRange(start, end, direction);
       },
       select: () => {
         inputFocusRef.current?.select();
       },

Committable suggestion was skipped due to low confidence.

nativeElement: holderRef.current.nativeElement || inputNumberDomRef.current,
}),
);
Expand Down
66 changes: 66 additions & 0 deletions tests/focus.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { fireEvent, render } from '@testing-library/react';
import InputNumber, { InputNumberRef } from 'rc-input-number';
import { spyElementPrototypes } from 'rc-util/lib/test/domHook';
import React from 'react';

const getInputRef = () => {
const ref = React.createRef<InputNumberRef>();
render(<InputNumber ref={ref} defaultValue={12345} />);
return ref;
};

describe('InputNumber.Focus', () => {
let inputSpy: ReturnType<typeof spyElementPrototypes>;
let focus: ReturnType<typeof jest.fn>;
let setSelectionRange: ReturnType<typeof jest.fn>;

beforeEach(() => {
focus = jest.fn();
setSelectionRange = jest.fn();
inputSpy = spyElementPrototypes(HTMLInputElement, {
focus,
setSelectionRange,
});
});

afterEach(() => {
inputSpy.mockRestore();
});

it('start', () => {
const input = getInputRef();
input.current?.focus({ cursor: 'start' });

expect(focus).toHaveBeenCalled();
expect(setSelectionRange).toHaveBeenCalledWith(expect.anything(), 0, 0);
});

it('end', () => {
const input = getInputRef();
input.current?.focus({ cursor: 'end' });

expect(focus).toHaveBeenCalled();
expect(setSelectionRange).toHaveBeenCalledWith(expect.anything(), 5, 5);
});

it('all', () => {
const input = getInputRef();
input.current?.focus({ cursor: 'all' });

expect(focus).toHaveBeenCalled();
expect(setSelectionRange).toHaveBeenCalledWith(expect.anything(), 0, 5);
});

it('disabled should reset focus', () => {
const { container, rerender } = render(<InputNumber prefixCls="rc-input-number" />);
const input = container.querySelector('input')!;

fireEvent.focus(input);
expect(container.querySelector('.rc-input-number-focused')).toBeTruthy();

rerender(<InputNumber prefixCls="rc-input-number" disabled />);
fireEvent.blur(input);

expect(container.querySelector('.rc-input-number-focused')).toBeFalsy();
});
});
Comment on lines +30 to +66
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. blur() 方法的测试
  2. select() 方法的测试
  3. 边界情况的处理(如空值时的光标位置)

建议添加如下测试用例:

it('blur', () => {
  const input = getInputRef();
  input.current?.focus();
  input.current?.blur();
  expect(blur).toHaveBeenCalled();
});

it('select', () => {
  const input = getInputRef();
  input.current?.select();
  expect(focus).toHaveBeenCalled();
  expect(setSelectionRange).toHaveBeenCalledWith(expect.anything(), 0, 5);
});

10 changes: 10 additions & 0 deletions tests/input.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,16 @@ describe('InputNumber.Input', () => {
expect(onChange).not.toHaveBeenCalled();
});

it('input should work', () => {
const ref = React.createRef<InputNumberRef>();
const { container } = render(<InputNumber ref={ref} />);
const inputEl = container.querySelector('input')!;
const rootEl = container.querySelector('.rc-input-number')!;

expect(ref.current?.input).toBe(inputEl);
expect(ref.current?.nativeElement).toBe(rootEl);
});

describe('nativeElement', () => {
it('basic', () => {
const ref = React.createRef<InputNumberRef>();
Expand Down