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: Step つき FormDialog の実装 #4972

Closed
wants to merge 7 commits into from

Conversation

schktjm
Copy link
Contributor

@schktjm schktjm commented Oct 2, 2024

関連URL

asana: https://app.asana.com/0/1206535203416259/1208178713972396/f
イメージ図: https://kufuinc.slack.com/archives/CGC58MW01/p1725506017398559?thread_ts=1725505216.026419&cid=CGC58MW01

概要

FormDialog に steppable="true" を渡すことで、ステップ付きダイアログを使えるようにします。

イメージ図

<FormDialog hasStep>
  <Step />
  <Step />
  <Step />
</FormDialog>

Step つきダイアログで実現したいこと

  • 次のステップへ移動したときに、フォーカスがダイアログのトップへ戻ること

変更内容

  • 2 step 以降にもう一度 <FocusTrap> にフォーカスが当たるように、 ref のフォワーディングをした
  • FormDialog に steppable: boolean の props を追加
  • step つきダイアログ用の useStepDialog.tsx を新規作成

確認方法

storybook の Form Dialog With Steps の動作確認をお願いします!

http://localhost:6006/?path=/story/dialog%EF%BC%88%E3%83%80%E3%82%A4%E3%82%A2%E3%83%AD%E3%82%B0%EF%BC%89-dialog--form-dialog-with-step

@schktjm schktjm self-assigned this Oct 2, 2024
Copy link

pkg-pr-new bot commented Oct 2, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/kufu/smarthr-ui@4972

commit: 846cd41

Comment on lines +343 to +349
onSubmit={(closeDialog, e, step) => {
action('executed')()
setResponseMessage(undefined)
if (step && step === 2) {
closeDialog()
}
}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

もやポイント1

closeDialog の実行は FormDialog の使用する側なので、使用する側で最後のページかのチェックをする必要があります!

Comment on lines 80 to 97
title={steppable ? `${title}${titleSuffix}` : title}
titleId={titleId}
subtitle={subtitle}
titleTag={titleTag}
contentBgColor={contentBgColor}
contentPadding={contentPadding}
actionText={actionText}
actionText={steppable ? getActionText(actionText) : actionText}
actionTheme={actionTheme}
actionDisabled={actionDisabled}
closeDisabled={closeDisabled}
subActionArea={subActionArea}
subActionArea={steppable ? renderSubActionButton() : subActionArea}
onClickClose={handleClickClose}
onSubmit={handleSubmitAction}
responseMessage={responseMessage}
decorators={decorators}
steppable={steppable}
>
{children}
{steppable ? childrenSteps[activeStep] : children}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

もやポイント2?

props 渡す箇所で三項演算子書き過ぎかもなぁと考えています!

Copy link
Contributor

Choose a reason for hiding this comment

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

steppablefalse のときでも actionTextactionText={getActionText(actionText)} としても動くから、まとめてしまっても良いかも?(stepが1個しか無いとして扱える)

Copy link
Contributor

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.

useStepDialog に hasStep を渡してあげて、hooks 内で同じ条件をかけばいけるんですけど、通常 FormDialog の actionText ロジックをカスタムフックス内で持っちゃうとわかりにくくなっちゃうかな〜と思ってきました!


import { FocusTrapRef } from './FocusTrap'

const NEXT_BUTTON_LABEL = '次へ'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

聞きたいポイント

次へのラベルは変えることがなさそうと思ったのですが、変更している例があれば教えてほしいです 🙏

Copy link
Contributor

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.

submit のボタンテキストは props.actionText で定義されているのに対して戻るボタンは props.decorators.closeButtonLabel() で渡せるようになっているので次へテキストをどうわたせるようにするか迷っています

Copy link
Contributor Author

Choose a reason for hiding this comment

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

一旦一番実直な方法で書いてみました!

0c6e6ef

@schktjm schktjm marked this pull request as ready for review October 2, 2024 11:16
@schktjm schktjm requested a review from a team as a code owner October 2, 2024 11:16
@schktjm schktjm requested review from s-sasaki-0529, hiroki0525 and masuP9 and removed request for a team October 2, 2024 11:16
Copy link
Contributor

@masuP9 masuP9 left a comment

Choose a reason for hiding this comment

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

いくつかコメントしましたが、動作は良さそう!

*/
onSubmit: (closeDialog: () => void, e: FormEvent<HTMLFormElement>, activeStep?: number) => void
/** Stepつきダイアログか否か */
steppable?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

[IMO] Step可能かどうか(steppable)、というより、Stepがあるかどうか(isSteps)、もしくはStepを持っているかどうか(hasSteps)の方が良さそうに思いました(hasSteps推し)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

steppable がひと単語で直感的に気に入りつつも、誤字ですか?って怒られ続けたのでたしかに Stepを持っているかとかのようが良い気がしました!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

653fd5a

命名変えました!

@schktjm schktjm requested a review from masuP9 October 8, 2024 04:33
getActionText,
handleNextSteps,
renderSubActionButton,
} = useStepDialog(children)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nits]
今から根本から作り変えることになってしまいますし、今のコードでもちゃんと動いていそうなのでnits程度にとどめておくのですが、 FormDialog とは別のコンポーネントに切り出すのが良いのかなぁ...と思いました。
理由としては次の感じです。

  • コンポーネントのインターフェースがちょっと特殊
    • propsが hasStep:true の場合のみ、次のページ数 のようにステップ付きかどうかで関数に渡される引数が変わる
    • 暗黙的に各ステップとして Dialog の子コンポーネントを下記のように配置されることが利用者側に要求される
<FormDialog>
  // これは一個目のステップ
  <Hoge />
  // これは二個目のステップ
  <Fuga />
</FormDialog>
  • ステップ付きダイアログ専用のロジックが混入する
    • 特に useStepDialog のロジックも hasStep を使わない場合は不要なhooksになってしまう。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

レビューありがとうございますー!全然アリだと思います!

定例で相談したとき、簡単に書けそうだったら FormDialog に hasStep 生やす程度でできたら嬉しいということを聞いたんですが、もやポイントに書いた通り onClose の発火タイミングを使用している側で調整したりする必要があったりで複雑そうな雰囲気があったので!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ちょっとPR別で作ってみます!

@hiroki0525
Copy link
Contributor

hiroki0525 commented Oct 10, 2024

[Q]
各ステップごとで onSubmit が走るのですがこれは想定通りだったでしょうか?
各ステップごとで onSubmit するのではなく、例えば各ステップを切り替えたときは onStepChange みたいなハンドラが発火して、最後のステップで「保存」ボタンを押すと onSubmit が発火する、という使い方もあるのかなと。

@schktjm
Copy link
Contributor Author

schktjm commented Oct 11, 2024

@hiroki0525

Form が数 step 分重なっているという認識だったので各 step ごとに onSubmit でもいいかな〜と思ったのですが、最後に処理を投げるという目線で見ると onSubmit は完了時だけのほうが良さそうですね!
それを考えるとやはり FormDialog に付け足すのはちょっと処理が複雑になってしまいそうです 🤔
それも合わせて考えてみます〜!

@schktjm
Copy link
Contributor Author

schktjm commented Oct 24, 2024

#5004 で進めていくことになったため close

@schktjm schktjm closed this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants