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

作品作成画面の対応拡張子のポップアップ #235

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

powaaaaa
Copy link
Collaborator

@powaaaaa powaaaaa commented May 6, 2024

issue番号

closes #212

変更点概要

  • itemsの中にInfoPopOverを作成しました
  • AssetUpload.tsxThumbnailUpload.tsxに追加しました

参考文献(あれば)

スクリーンショット(必要であれば)

スクリーンショット 2024-05-07 1 17 53
スクリーンショット 2024-05-07 1 18 11

チェック項目

  • テストが通ったか
  • ビルドが通ったか
  • self assignしたか
  • レビュー優先度のラベルをつけたか

@powaaaaa powaaaaa self-assigned this May 6, 2024
Copy link

vercel bot commented May 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
toy-box-front-replace ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 6, 2024 8:36am

@@ -23,27 +24,10 @@ export const AssetUpload: FC<Props> = ({ handleUploadAssets, assets }) => (
<Typography className="text-red-500" variant="body2">
必須
</Typography>
<InfoPopOver>
{`対応形式:\n 画像[.png .jpg .jpeg .bmp .gif]\n 動画[.mp4 .mov]\n 音源[.mp3 .wav .m4a ]\n モデル[.gltf .fbx]\n zip[.zip]`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

文字の前後の空白を入れるか入れないかで統一してほしいです
[.xxx] or [ .xxx ]
前者なら以下の修正

Suggested change
{`対応形式:\n 画像[.png .jpg .jpeg .bmp .gif]\n 動画[.mp4 .mov]\n 音源[.mp3 .wav .m4a ]\n モデル[.gltf .fbx]\n zip[.zip]`}
{`対応形式:\n 画像[.png .jpg .jpeg .bmp .gif]\n 動画[.mp4 .mov]\n 音源[.mp3 .wav .m4a]\n モデル[.gltf .fbx]\n zip[.zip]`}

@@ -23,27 +24,10 @@ export const AssetUpload: FC<Props> = ({ handleUploadAssets, assets }) => (
<Typography className="text-red-500" variant="body2">
必須
</Typography>
<InfoPopOver>
{`対応形式:\n 画像[.png .jpg .jpeg .bmp .gif]\n 動画[.mp4 .mov]\n 音源[.mp3 .wav .m4a ]\n モデル[.gltf .fbx]\n zip[.zip]`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

<br />じゃだめ?(特に要件とか仕様は確認してないので,何か理由があるなら無視してOK.)

Suggested change
{`対応形式:\n 画像[.png .jpg .jpeg .bmp .gif]\n 動画[.mp4 .mov]\n 音源[.mp3 .wav .m4a ]\n モデル[.gltf .fbx]\n zip[.zip]`}
対応形式:
<br />
画像[.png .jpg .jpeg .bmp .gif]
<br />
動画[.mp4 .mov]
<br />
音源[.mp3 .wav .m4a ]
<br />
モデル[.gltf .fbx]
<br />
zip[.zip]

children: ReactNode;
};

export const InfoPopOver: FC<Props> = ({ children }) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Infoだと情報が薄くて何の情報なのかわかりづらいからSupportExtPopOverとかにしてもいいかも

@@ -23,27 +24,10 @@ export const AssetUpload: FC<Props> = ({ handleUploadAssets, assets }) => (
<Typography className="text-red-500" variant="body2">
必須
</Typography>
<InfoPopOver>
{`対応形式:\n 画像[.png .jpg .jpeg .bmp .gif]\n 動画[.mp4 .mov]\n 音源[.mp3 .wav .m4a ]\n モデル[.gltf .fbx]\n zip[.zip]`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

拡張子情報は親要素から受け取るわけじゃないからpopoverの中に隠蔽しちゃっていいと思う

Copy link
Collaborator

Choose a reason for hiding this comment

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

って思ったけどこれアセットとサムネイルで違うのか

Comment on lines 22 to 24
<TooltipContent className="rounded-md relative text-xl border-2 bg-white border-orange-pop">
<p className="whitespace-pre-wrap text-[10px] font-bold">{children}</p>
</TooltipContent>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<TooltipContent className="rounded-md relative text-xl border-2 bg-white border-orange-pop">
<p className="whitespace-pre-wrap text-[10px] font-bold">{children}</p>
</TooltipContent>
<TooltipContent className="rounded-md relative text-xl border-2 bg-white border-orange-pop">
{
supportedExts.map(({category,exts})=>(
<Vertical>
<Typography>
{category}
</Typography>
<span>
[
<Vertical>
{
exts.map((ext)=>(
<p>
.{ext}
</p>
))
}
<Vertical>
]
</Vertical>
))
}
</TooltipContent>

冗長ではあると思うけど私だったらこういうふうに書くかも(動くかどうかは確かめてない、あくまでイメージ)

supportedExtっていうpropsを取ってそれを元にPopoverがレンダリングする

supportedExtはcategoryextsをkeyに持つ構造体の配列

で親からsupportedExtを渡すだけでpopoverが統一されたスタイルでレンダリングをしてくれる

親が渡すsupportedExtは固定値で持っておけば何か変更があったとしても構造体を変えるだけでそれまでと同様のスタイルが保てる(文字列だと開発者がスタイルを気にして開発しなきゃいけなくなる(ドットだったりスペースだったり))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ありがとうございます 勉強になりま
そんな感じにしてみます

@powaaaaa
Copy link
Collaborator Author

powaaaaa commented May 7, 2024

こんな感じになりました。よろしくお願いしますす

スクリーンショット 2024-05-07 16 15 18
スクリーンショット 2024-05-07 16 14 58

@powaaaaa powaaaaa requested review from tosaken1116 and K-Kizuku May 7, 2024 07:16
@@ -23,27 +24,31 @@ export const AssetUpload: FC<Props> = ({ handleUploadAssets, assets }) => (
<Typography className="text-red-500" variant="body2">
必須
</Typography>
<SupportExtPopOver
supportedExts={[
Copy link
Collaborator

Choose a reason for hiding this comment

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

supportedExtsは別で変数化して渡すだけにする
src/contstantsに記述する
Thumbnailも同様の変更が必要

メリット
対応拡張子が増えたときに編集する箇所がconstantsだけになりuiファイルに変更が起きないため意図しない変更が起きづらい

もし可能であればinputのaccept fileもsupportedExtsを参照するようにしたい(これは別issueに切り出しても良き)

https://github.com/Kyutech-C3/ToyBox_front_replace/blob/d0bb9a5915eb678c6c15ddaddc7f6861686ad48b/src/domains/Work/components/WorkEdit/presentations/items/AssetUpload.tsx#L70C4-L82C9

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

了解です
とりあえずsupportedExtsはやります
inputの方はついででできる感じならしますけどissue切り出す?

Copy link
Collaborator

Choose a reason for hiding this comment

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

個人的にはissueの内容と外れるので分けた方がいいとは思いますけどそこまで大きいタスクでもないのでやりやすい方で任せます

@powaaaaa
Copy link
Collaborator Author

powaaaaa commented May 7, 2024

assetのaccept fileもできました
constantsのファイルの作り方よくわからんかったのでダメやったら教えてください。
ポップオーバーのところはちゃんと動いてました

acceptのとこはどう確認したらいいかわからんかったけど、ぱっと見gifは良くてpdfだめってなってるからちゃんと動いてそうと思いました

スクリーンショット 2024-05-07 21 27 34

よろお願いします

@powaaaaa
Copy link
Collaborator Author

@K-Kizuku @tosaken1116

Comment on lines 24 to 25
export const ASSET_ACCEPT_EXTENSIONS =
'image/png, image/jpeg, image/jpg, image/bmp, image/gif, video/mp4, video/mov, audio/mp3, audio/wav, audio/m4a, model/gltf, model/fbx, application/zip';
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここは静的なものではなくASSET_EXTENSIONSから生成するようにしてください

@powaaaaa powaaaaa requested a review from tosaken1116 June 7, 2024 02:27
@powaaaaa
Copy link
Collaborator Author

powaaaaa commented Jun 7, 2024

お願いします
@K-Kizuku @tosaken1116

? `${assets.length}件のアセットをアップロード済み`
: 'アセットをアップロードしてください'}
export const AssetUpload: FC<Props> = ({ handleUploadAssets, assets }) => {
console.log('asset: ', ASSET_ACCEPT_EXTENSIONS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

console.logは消してください

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ごめんなさい

category: 'zip',
exts: ['zip'],
},
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

as constをつけてください

Copy link
Collaborator

Choose a reason for hiding this comment

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

category: '画像',
exts: ['png', 'jpg', 'jpeg', 'bmp', 'gif'],
},
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

as const

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

作品作成画面の対応形式拡張子をポップアップに変更
3 participants