chore: FormDialogの内部処理を整理する #5321
Conversation
yagimushi
force-pushed
the
chore-refactoring-FormDialog
branch
6 times, most recently
from
b4642a8 to
67f0735
Compare
commit: |
yagimushi
force-pushed
the
chore-refactoring-FormDialog
branch
from
67f0735 to
4364bcd
Compare
yagimushi
force-pushed
the
chore-refactoring-FormDialog
branch
from
7ae0086 to
d77fb16
Compare
| @@ -24,7 +24,7 @@ export const FormDialog: React.FC<Props & ElementProps> = ({ | |||
| onClickClose, | |||
| onPressEscape = onClickClose, | |||
| responseMessage, | |||
| actionDisabled = false, | |||
| actionDisabled, | |||
There was a problem hiding this comment.
actionDisabledは最終的にButtonのdisabledとして設定されるだけであり、falseを指定する意味が薄かったため、初期化しないように修正しました
| if (!props.isOpen) { | ||
| return | ||
| if (props.isOpen) { | ||
| onClickClose() |
There was a problem hiding this comment.
早期returnしていましたが
- 直後の実行の条件を逆転させているだけであり、コードの理解が一手遅れる
- !とreturn分処理が無駄になっている
というデメリットのほうが大きそうだったので調整しています
| const calcedResponseStatus = useMemo(() => { | ||
| if (!responseMessage) { | ||
| return { | ||
| isProcessing: false, | ||
| visibleMessage: false, | ||
| } | ||
| } | ||
|
|
||
| if (responseMessage.status === 'processing') { | ||
| return { | ||
| isProcessing: true, | ||
| visibleMessage: false, | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| isProcessing: false, | ||
| visibleMessage: true, | ||
| // HINT: statusがprocessingではない === success or errorであることが確定する | ||
| // success or error の場合、text属性も必ず存在する | ||
| status: responseMessage.status as 'success' | 'error', | ||
| message: (responseMessage as { text: string }).text, | ||
| } | ||
| }, [responseMessage]) |
There was a problem hiding this comment.
responseMessageの取り扱いが煩雑になっているので、扱いやすい形に変換、memo化するようにしました。
他コンポーネントでも同様の変換処理が有るため、別PRでカスタムhookを作成する予定です
AtsushiM
requested review from
yt-ymmt and
uknmr
and removed request for
a team
| @@ -63,11 +65,11 @@ export const FormDialogContentInner: FC<FormDialogContentInnerProps> = ({ | |||
| contentBgColor, | |||
| contentPadding, | |||
| actionText, | |||
| actionTheme = 'primary', | |||
There was a problem hiding this comment.
こちらの primary を外すと、Button側のdefault値が secondary になっているため、actionTheme を渡さない使い方をしているページでボタンカラーが変わってしまいそうでした。
https://github.com/kufu/smarthr-ui/blob/master/packages/smarthr-ui/src/components/Button/Button.tsx#L56
There was a problem hiding this comment.
@yt-ymmt
初期化のタイミングを移動しています。
https://github.com/kufu/smarthr-ui/pull/5321/files#diff-832b6494bc9958dd0b5533e5cc577f0096ced1073e4d055e5ad28040b697d639R200
実際に設定される前に初期化されてしまうとバグる可能性があり得るので...
|
|
||
| const { form, wrapper, actionArea, buttonArea, message } = formDialogContentInner() | ||
| const calcedResponseStatus = useMemo(() => { |
There was a problem hiding this comment.
calcedResponseStatus は何かのtypoですかね？
…e-refactoring-FormDialog
関連URL
概要
変更内容
確認方法