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: Textareaの文字数カウンタをonChangeで処理するように修正 #4916

Merged
merged 5 commits into from
Sep 13, 2024
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ export default {
}

const Template: StoryFn = () => {
const [value, setValue] = useState('message👌')
const onChangeValue = (e: React.ChangeEvent<HTMLTextAreaElement>) =>
setValue(e.currentTarget.value)
const [value1, setValue1] = useState('message👌')
const [value2, setValue2] = useState('message👌')
const [value3, setValue3] = useState('message👌')
Comment on lines +17 to +19
Copy link
Contributor Author

Choose a reason for hiding this comment

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

同一のstateが利用されていたため、1つのコンポーネントで変更したものが他に影響を与えていたため修正しました。

return (
<ListStack>
<li>
Expand Down Expand Up @@ -58,8 +58,8 @@ const Template: StoryFn = () => {
<Textarea
name="max_length_with_value"
maxLetters={140}
value={value}
onChange={onChangeValue}
value={value1}
onChange={(e) => setValue1(e.target.value)}
/>
</FormControl>
</li>
Expand All @@ -68,8 +68,8 @@ const Template: StoryFn = () => {
<Textarea
name="max_length_with_value_over"
maxLetters={4}
value={value}
onChange={onChangeValue}
value={value2}
onChange={(e) => setValue2(e.target.value)}
/>
</FormControl>
</li>
Expand All @@ -78,8 +78,8 @@ const Template: StoryFn = () => {
<Textarea
name="max_length_with_value_and_decorators"
maxLetters={140}
value={value}
onChange={onChangeValue}
value={value3}
onChange={(e) => setValue3(e.target.value)}
decorators={{
beforeMaxLettersCount: (txt) => `entry limit(${txt})`,
afterMaxLettersCount: (txt) => ` characters(${txt})`,
Expand Down
30 changes: 21 additions & 9 deletions packages/smarthr-ui/src/components/Textarea/Textarea.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, {
ComponentPropsWithRef,
forwardRef,
startTransition,
useCallback,
useEffect,
useImperativeHandle,
Expand All @@ -12,6 +13,7 @@ import { tv } from 'tailwind-variants'

import { useId } from '../../hooks/useId'
import { useTheme } from '../../hooks/useTailwindTheme'
import { debounce } from '../../libs/debounce'
import { defaultHtmlFontSize } from '../../themes/createFontSize'

import type { DecoratorsType } from '../../types'
Expand Down Expand Up @@ -99,6 +101,7 @@ export const Textarea = forwardRef<HTMLTextAreaElement, Props & ElementProps>(
onInput,
decorators,
error,
onChange,
...props
},
ref,
Expand Down Expand Up @@ -135,15 +138,24 @@ export const Textarea = forwardRef<HTMLTextAreaElement, Props & ElementProps>(
}
}, [autoFocus])

const onKeyUp = useMemo(
() =>
maxLetters
? (event: React.KeyboardEvent<HTMLTextAreaElement>) => {
setCount(getStringLength(event.currentTarget.value))
}
: undefined,
[maxLetters],
// eslint-disable-next-line react-hooks/exhaustive-deps
const debouncedUpdateCount = useCallback(
debounce((value) => {
startTransition(() => {
setCount(getStringLength(value))
})
Copy link
Contributor

Choose a reason for hiding this comment

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

ask

startTransition についてあまり知らないので教えてほしいです!

これは関数内での状態更新の優先度を下げることで UI のブロッキングを解消してひっそりと更新する用途という理解なんですが、この機能があれば debouce せずとも元々の課題だったUIのカクつきを回避しながら onChange で直越更新できるってこと無いですかね…?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

これは関数内での状態更新の優先度を下げることで UI のブロッキングを解消してひっそりと更新する用途という理解

私も同じような理解をしています(存在は知ってたけど使ったことなかった)

debouce せずとも元々の課題だったUIのカクつきを回避しながら onChange で直越更新できるってこと無いですかね…?

可能性としてはありそうですが、setCountの実行回数自体は変わらないはずなのでやはりパフォーマンス懸念は残ってしまうのかも?とか想像しましたが、自信はないです...

@hiroki0525 nukosuke-sanお詳しそうなので、このあたりご意見や知見ありますか? 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

これは関数内での状態更新の優先度を下げることで UI のブロッキングを解消してひっそりと更新する用途という理解

自分も同じ理解です 😇

この機能があれば debouce せずとも元々の課題だったUIのカクつきを回避しながら onChange で直越更新できるってこと無いですかね…?

もしかしたら startTransition だけでも十分かもしれませんね...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

お二人ともありがとうございます〜!一旦はこちらの実装でいこうと思うんですが、リファクタ可能かもということで認識しておきます...🙇 !
startTransition解像度高めたい

}, 200),
[],
)

const handleChange = useCallback(
(event: React.ChangeEvent<HTMLTextAreaElement>) => {
onChange && onChange(event)
Copy link
Contributor

Choose a reason for hiding this comment

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

ask

ここ onChange 呼び出し結果の真偽値まで見てるのどういう意図あるんですっけ…?

Copy link
Contributor Author

@atzzCokeK atzzCokeK Sep 13, 2024

Choose a reason for hiding this comment

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

@s-sasaki-0529
TextareaのonChangeの定義が以下のようになっているので、onChangeがundefinedの可能性があるからこのような実装にしています〜 🙇

React.ChangeEventHandler | undefined

maxLetters && debouncedUpdateCount(event.currentTarget.value)
},
[debouncedUpdateCount, maxLetters, onChange],
)

const handleInput = useCallback(
(e: React.ChangeEvent<HTMLTextAreaElement>) => {
if (!autoResize) {
Expand Down Expand Up @@ -188,7 +200,7 @@ export const Textarea = forwardRef<HTMLTextAreaElement, Props & ElementProps>(
{...props}
{...textareaStyleProps}
aria-describedby={actualMaxLettersId}
onKeyUp={onKeyUp}
onChange={handleChange}
ref={textareaRef}
aria-invalid={error || undefined}
rows={interimRows}
Expand Down
26 changes: 26 additions & 0 deletions packages/smarthr-ui/src/libs/debounce.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

パッケージを導入せず独自実装した理由は、要件がシンプルだったため、わざわざ外部ライブラリに依存する必要がなく、独自実装で十分そうと判断したためです。

またlodash.debounce を用いた実装も、lodash.mergeやlodash.range同様に導入してみました。
ですが、ビルド時に以下のようなエラーが発生し、解決に時間がかかりそうだったので中断しました。

src/libs/lodash.ts:7:14 - error TS2742: The inferred type of 'debounce' cannot be named without a reference to '.pnpm/@[email protected]/node_modules/@types/lodash'. This is likely not portable. A type annotation is necessary.

7 export const debounce = _debounce

Copy link
Contributor

Choose a reason for hiding this comment

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

全然スコープ外の話をするんですけど、この debounce 関数をプロダクトでも使いたい…。
任意のライブラリ導入したり、これみたいに自前で実装すれば良い話ではあるんですが、せっかくなら社内全体で汎用的に使える状態になってると嬉しいなって思いました。tamatebako 案件かも。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s-sasaki-0529 実際にtamatebakoに入ったら使われそうですか?:eyes:
もしニーズ有りそうであればあればtamatebakoに移そうかな〜と思います!tokky-sanと話して進めていくべきなんですかね?tamatebako全然見てなかったので、どなたかの許可とかいるのか気になっています。 @Tokky0425

Copy link
Collaborator

Choose a reason for hiding this comment

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

OSS なので、PR 投げましょう!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ぶん投げまーす🥋🔥

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* 関数の呼び出しを指定された待機時間 (wait) だけ遅延させるデバウンス関数。
* 直前に同じ関数が呼び出された場合、タイマーをリセットして再び遅延させます。
*
* @param {T} func - 実行する関数
* @param {number} wait - デバウンスの待機時間(ミリ秒)
* @returns {(...args: Parameters<T>) => void} デバウンスされた関数
*
* @example
* const debouncedFunction = debounce(() => console.log('Called!'), 200);
*/
export const debounce = <T extends (...args: any[]) => void>(
func: T,
wait: number,
): ((...args: Parameters<T>) => void) => {
let timeoutId: ReturnType<typeof setTimeout> | null = null

return function (...args: Parameters<T>) {
if (timeoutId !== null) {
clearTimeout(timeoutId)
}
timeoutId = setTimeout(() => {
Copy link
Contributor

@hiroki0525 hiroki0525 Sep 13, 2024

Choose a reason for hiding this comment

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

setTimeout 使うなら requestIdleCallbackschedular.postTask あたりを使ったほうがよりベターですが、対応していないブラウザの考慮が必要&そこまでパフォーマンスチューニングしなくても良いと思うので setTimeout で良さそう!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hiroki0525 ガーサス、詳しい...!!! nukosuke-sanの自作ライブラリいずれ入れましょう! 🔥

func(...args)
}, wait)
}
}