Conversation
…fund-page # Conflicts: # view/next-project/package-lock.json # view/next-project/package.json # view/next-project/src/generated/hooks.ts # view/next-project/src/generated/model/index.ts # view/next-project/src/generated/model/teacher.ts
壊れたパスを参照してたため
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Deploying finansu with
|
| Latest commit: |
a5a4479
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e54a8c26.finansu.pages.dev |
| Branch Preview URL: | https://feat-chikuwa-add-fund-page.finansu.pages.dev |
There was a problem hiding this comment.
Code Review
This pull request implements the 'Campus Fund' feature, adding components for donation reporting and teacher selection, a new dashboard page, and the react-datepicker dependency. It also updates project configurations, including a Prettier upgrade and VS Code settings. Feedback focuses on removing a debug console.log, correcting invalid Tailwind CSS classes, improving type safety by using generics and avoiding unsafe type casting, and optimizing performance by memoizing floor option calculations.
| const [selectedBuilding, setSelectedBuilding] = useState( | ||
| null as CampusFundBuildingSummary | null, | ||
| ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
意図的にこの書き方をしている。
useState<T | null>(null) が保存時の自動整形で useState(null) に崩れるため、回避として null as T | null を使っています。selectedTeacher も同様。
There was a problem hiding this comment.
募金ページを開いた直後は建物・教員が未選択だからnull許容してる。
あと自動整形で変になっちゃう問題治ってたから提案通りに修正
| // TODO APIのレスポンスを修正してyearIdをちゃんと取得できるようにする | ||
| type YearData = { id: number; year: number; createdAt: string; updatedAt: string }; | ||
|
|
||
| const years = yearData.data as unknown as YearData[]; |
There was a problem hiding this comment.
年度取得API修正時に治す。
TODOとしている
| }, | ||
| ]; | ||
|
|
||
| const floorOptions = [...new Set(teachers.map((teacher) => teacher.floorNumber))]; |
There was a problem hiding this comment.
API繋ぎこみ前のモックデータのためこのままとする
hikahana
left a comment
There was a problem hiding this comment.
先にちゃんとスキーマ作ってから一緒に開発すればよかったなー
繋ぎ込みの時にスキーマ周りの修正必要かも。
コードだけチラ見でレビューです
| if (!teacher || !formData.receivedAt || !formData.price || !user?.id || !yearId) return; | ||
|
|
||
| try { | ||
| setErrorMessage(''); |
There was a problem hiding this comment.
[imo]
これ初期化って感じ?
それなら、useStateの宣言時に空を入れてるから不要かも。
There was a problem hiding this comment.
エラー→再度登録の際にエラーメッセージが出たままになってしまうため、登録処理開始時に初期化するようにしている
There was a problem hiding this comment.
あああああああああああああああああああああ
その動きがあったか
| setErrorMessage(''); | ||
| setIsSubmitting(true); | ||
|
|
||
| const payload: CreateCampusDonationPayload = { |
There was a problem hiding this comment.
prettier怒られてるね。
payload特に使ってないし、CreateCampusDonationPayloadだけの呼び出しでもいいのかも?
await CreateCampusDonationPayload
みたいな感じってエラー起きるっけ?
There was a problem hiding this comment.
ESLint止めてて見落とし
PayLoad削除
API繋ぎこみのときいいかんじにする
| }); | ||
| handleClose(); | ||
| alert('募金の登録が完了しました!'); | ||
| } catch (error) { |
There was a problem hiding this comment.
ここもprettierだね。error使ってないし、なくてもいいんでない?
There was a problem hiding this comment.
ESLint止めてて見落とし
error使っていないため削除
|
|
||
| <div className='flex items-center gap-3'> | ||
| <label className='w-20 shrink-0 text-right text-xs font-bold text-gray-600 md:text-sm'> | ||
| 記入担当者 |
There was a problem hiding this comment.
[ask]
ここって名前だったけ?user_idが入るようになったとかじゃなかったけ?
認識違ってたらごめんね
There was a problem hiding this comment.
表示用にuser.nameを出してるけど、登録時にAPIに送るのはuserid
| className='w-full border-b border-gray-400 bg-gray-100 text-sm focus:outline-none md:text-base' | ||
| /> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
ここ一行消しといてーー
インデントである程度わかると思うから下にある改行も消していいかも
| const ReportModal = ({ isOpen, onClose, building, teacher, yearId, onBack }: Props) => { | ||
| const initialFormData: CampusFundFormData = { | ||
| receivedAt: new Date(), | ||
| price: '', |
There was a problem hiding this comment.
[ask]
priceって数値じゃなくて文字列なのはなんでだろう?
There was a problem hiding this comment.
文字列として仮のデータを作っていたことが原因
他のページ参考に数値に修正
| } | ||
|
|
||
| const OpenEditModalButton = (props: Props) => { | ||
| const [showModal, setShowModal] = useState(false); |
There was a problem hiding this comment.
showModal使ってないね〜
使わない想定なら↓でいいよーー
const [_, setShowModal] = useState(false)
There was a problem hiding this comment.
ESLint止めてて見落としてた
このファイル使ってないのに残っていたので削除しました
| > | ||
| 戻る | ||
| </button> | ||
| <PrimaryButton onClick={handleSubmit} disabled={isSubmitDisabled || isSubmitting}> |
There was a problem hiding this comment.
[ask]
isSubmitDisabledってisSubmittingと同じことしてる感じがしているんだけど分けた理由とかある?
もし同じならisSubmittingだけでもいいかなーって
There was a problem hiding this comment.
入力データの不足により送信ボタンを押せない(isSubmitDisabled)と送信中だから送信ボタンを押せない(isSubmitting)で分けてる
あと登録ボタンを「登録する」から「登録中」の表示に変えたり
| // フォーム入力中のstate用 | ||
| export interface CampusFundFormData { | ||
| receivedAt: Date | null; | ||
| price: string; |
| const [selectedBuilding, setSelectedBuilding] = useState( | ||
| null as CampusFundBuildingSummary | null, | ||
| ); |
Co-authored-by: Copilot <copilot@github.com>
概要
/campus_fundを追加useStateの型定義が保存時に崩れる問題に対応/teachersがエラーになるため、教員一覧リンクを一時的に無効化画面スクリーンショット等
テスト項目
備考