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

ODY-293 Introduce Storybook and add MainCard #11

Merged
merged 17 commits into from
Jan 10, 2024
Merged

Conversation

tom-takeru
Copy link
Contributor

@tom-takeru tom-takeru commented Dec 8, 2023

事前確認

  • PR前に動作確認をしたか
  • 機密情報を含んでいないか
  • 開発ルールを守って実装しているか
  • 単体テストが全てOKになっているか(単体テストを作成していないリポジトリの場合SKIP)
  • 最新のdevelopブランチを反映しているか
  • ビルドエラー/ESLintエラーは起きていないか

レビュー優先度(どれか一つ)

  • すぐ見てもらいたい
  • 3日くらいで見てもらいたい
  • 7日くらいで見てもらいたい

レビューの度合い

  • コードをぱっと確認してもらいたい
  • コードをぱっと確認 & 動作確認してもらいたい
  • コードをしっかり確認 & 動作確認してもらいたい

レビュワーによる動作確認

やったこと

  • Storybookの導入
  • MainCardの実装

やらないこと

Storyのファイルとコンポーネントのファイルを同じフォルダに配置すること(現状はstoriesフォルダにストーリーファイルがある)

懸念点や注意点

他の開発のブロッカーにもなっているのではやめにレビューをお願いします。

その他

Storybookの動作確認方法

  1. npm run storybookで起動
  2. storybookタブを押下

@tom-takeru tom-takeru marked this pull request as ready for review December 13, 2023 10:06
module.exports = config;
defaultConfig.resolver.resolverMainFields.unshift('sbmodern');

module.exports = defaultConfig;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"format:fix": "prettier --write .",
"storybook-generate": "sb-rn-get-stories",
"storybook-watch": "sb-rn-watcher",
"storybook": "sb-rn-get-stories && STORYBOOK_ENABLED='true' expo start"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

alignItems: 'center',
justifyContent: 'center',
borderTopLeftRadius: 25,
borderTopRightRadius: 25,
Copy link
Contributor Author

@tom-takeru tom-takeru Dec 14, 2023

Choose a reason for hiding this comment

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

[メモ]ReactNativeでは四隅に個別の角丸を指定する方法を解説(応用編)ができないらしい

Copy link
Contributor

@kzk27 kzk27 left a comment

Choose a reason for hiding this comment

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

実装とStorybookの導入ありがとうございます🙇
概ね問題ないのですが、少し質問があるのでコメント残してます🙏

.storybook/stories/tree/MainCard.stories.tsx Show resolved Hide resolved
app/_layout.tsx Show resolved Hide resolved
Comment on lines 12 to 15
<InputHour label='Time' />
<InputHour label='Time' />
<InputHour label='Time' />
<InputHour label='Time' />
Copy link
Contributor

Choose a reason for hiding this comment

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

[Q] こちらの入力フォームってWebの方だと全て同じコンポーネント(MainCartInput)で実装したのですが、今回時間用のコンポーネントに絞った理由って何かあるのでしょうか?🙏

Copy link
Contributor Author

@tom-takeru tom-takeru Dec 26, 2023

Choose a reason for hiding this comment

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

たしかにそうですね、、、。
Figmaで分けて作っていたので(添付)、なんとなく分けて作っていました、、、。

と思ったんですが、単位の位置が前か後ろか?みたいなことを考えてこうしてたっぽいですね。
全部propsにしてもう少し汎用的なコンポーネントにしてみます!

  • InputHour.tsx→Input.tsx
image

@kzk27
Copy link
Contributor

kzk27 commented Dec 23, 2023

@tom-takeru
上記に加えて一点確認させてください🙏

モバイル版のStorybookの使い方を以前画面共有しながら教えていただいたかと思うのですが、資料などはどこかに
まとめられてたりしますでしょうか?(あれば便利だなぐらいに思っているだけなのでもし無ければバックログに資料作りをチケット化するのも良いかなと思いました🙇)

.eslintignore Outdated
@@ -0,0 +1,3 @@
*
Copy link
Contributor

Choose a reason for hiding this comment

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

eslintignoreは廃止されたみたいな記事みたんだけど、使わないほうが良かったりするかなぁ
https://qiita.com/satoruk/items/48627a86d3ac51eff43b

Copy link
Contributor Author

@tom-takeru tom-takeru Dec 26, 2023

Choose a reason for hiding this comment

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

軽くこのファイル使わないやり方調べてみて時間かかりそうなら別チケットで起票しようかな。

  • .eslintignoreファイルを使わずにlintする対象フォルダを指定する方法を調査

@tom-takeru
Copy link
Contributor Author

tom-takeru commented Dec 26, 2023

@kzk27

モバイル版のStorybookの使い方を以前画面共有しながら教えていただいたかと思うのですが、資料などはどこかに
まとめられてたりしますでしょうか?

とりあえず各コマンドの使い方を簡単にREADME.md作って記載してみますね!

  • README.md作成

このPRにおける動作確認の方法は説明欄のその他の部分に追記しておきました!

@tom-takeru
Copy link
Contributor Author

tom-takeru commented Dec 26, 2023

  •  コンフリクト解消

@tom-takeru tom-takeru closed this Dec 26, 2023
@tom-takeru tom-takeru reopened this Dec 26, 2023
@Mellbrother
Copy link
Contributor

動作確認した
良い感じだね

少し気になる点あったから、記載する

  • storybookを起動した時にstorybook.require.jsに差分が表示されたんだけど、なんでかわかる(' → "とかインデントとかで、prettier周りかなと思いつつ、他のファイルでは保存してもそうならないから、確認)?
  • Mainカードのタイトルの横の白丸っていらないんだっけ(figmaにはある)?

@tom-takeru
Copy link
Contributor Author

@Mellbrother

storybookを起動した時にstorybook.require.jsに差分が表示されたんだけど、なんでかわかる

こっちのローカルでも差分出てた、、、。コミットしてプッシュしたんだけどどう?

Mainカードのタイトルの横の白丸っていらないんだっけ(figmaにはある)?

忘れてた。入れといた!

Copy link
Contributor

@kzk27 kzk27 left a comment

Choose a reason for hiding this comment

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

ご対応ありがとうございます!
LGTMです🙏

@tom-takeru tom-takeru merged commit 3fa6207 into develop Jan 10, 2024
@tom-takeru tom-takeru deleted the feature/ODY-293 branch January 10, 2024 04:57
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