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

注文画面ホームページを実装しました。 #62

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

Conversation

hata0
Copy link
Collaborator

@hata0 hata0 commented Dec 22, 2022

概要

#49 の内容を実装を実装しました。

デバッグ項目

  • コンソールに警告はありません。
  • 外観に大きな違和感はありません。
  • レスポンシブできています。

スクリーンショット

Before After

参考 URL

Self-Checking points 🚨

レビューを依頼する前に必ず確認してほしいポイントです。
一般的な項目を上げているので、プロジェクト毎に必要なポイントがあれば各リポジトリで追加してください。

共通 (命名)

  • data, info, value などの汎用的で抽象度の高い変数名を使っていない → 参考
  • 配列には末尾に s をつけて複数形にするか xxxList と命名することで配列であることを明確にしている → 参考
  • マジックナンバーは極力存在せず、定数を用いて表現されている 参考
  • パッと見で何をやってるかわからないような処理の結果を 説明変数 している → 参考
  • 複雑な条件式の結果を 要約変数 を適用している → 参考

共通 (処理系)

  • 早期リターン(ガード節) を適用することで条件分岐の簡略化している → 参考
  • if 文では「調査対象」を左側に、「比較対象」を右側に配置している → 参考
  • 値のパターンによって分岐する場合は is/else文 ではなく、switch文 を使う → 参考

共通 (コメント系)

  • コメントには適切なアノテーションコメントが記載されている → 参考

JavaScript (命名)

  • 変数名, 関数名, プロパティ名 は ローワーキャメルケース になっている → 参考
  • クラス名, オブジェクト名 は パスカルケース になっている → 参考
  • 定数名は スネークケース になっている → 参考

JavaScript (処理系)

  • 変数の宣言には極力を const を利用している → 参考
  • 文字列の合成にはテンプレートリテラルを利用している → 参考
  • ループ処理は 通常for文, for-in, forEach などは極力利用せず、高階関数を利用している → 参考
  • ==(等価演算子) は使わず ===(厳密等価演算子) を利用している → 参考

TypeScript (命名)

  • インターフェース, タイプ, Enum の命名は パスカルケース になっている → 参考

React (処理系)

  • コンポーネントの変数名は パスカルケース になっている → 参考
  • イベント処理のロジックは JSX 内に直接かかず、コールバック関数として JSX 外で定義する → 参考
  • コンポーネントに子要素がない場合は 自己終了タグ を利用する → 参考
  • attribute にブール値を直接記載する場合は省略形を利用する → 参考
  • attribute に文字列を直接記載する場合は波括弧を利用しない → 参考

共通 (その他)

  • フォーマット差分などは PR 上に一言 インラインコメント を付けて、レビュワーが省エネできるように → 参考

@vercel
Copy link

vercel bot commented Dec 22, 2022

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

Name Status Preview Comments Updated (UTC)
food-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 6, 2023 11:40am

Copy link
Contributor

@umiteru2004 umiteru2004 left a comment

Choose a reason for hiding this comment

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

よろしくお願いします!

<ul>
{Object.values(OrderContents).map((content) => {
return (
<li className="relative mb-[16px] pt-[18px] pb-[15px] pr-[14px] pl-[11px] w-full h-[94px] bg-navajowhite-a100 rounded-[13px]">
Copy link
Contributor

Choose a reason for hiding this comment

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

keyをお願いします!

next-dev.js?3515:20

Warning: Each child in a list should have a unique "key" prop.

Check the render method of `Order`.
See https://reactjs.org/link/warning-keys for more information.
    at li
    at Order (webpack-internal:///./pages/[shopId]/order.tsx:63:62)
    at MyApp (webpack-internal:///./pages/_app.tsx:14:27)
    at ErrorBoundary (webpack-internal:///./node_modules/next/dist/compiled/@next/react-dev-overlay/dist/client.js:8:20742)
    at ReactDevOverlay (webpack-internal:///./node_modules/next/dist/compiled/@next/react-dev-overlay/dist/client.js:8:23635)
    at Container (webpack-internal:///./node_modules/next/dist/client/index.js:70:9)

Copy link
Contributor

Choose a reason for hiding this comment

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

 現状だと、同じ名前の商品が2つ以上あるとkeyが重複してしまう。サービスが利用される上で、名前が同じ商品が2つ以上ある状態がないとも言い切れない気がするので、content.product.namekeyにするのは避けるべきかも。
 ただ、今回のケースは、バックエンド側で各商品にユニークなIDを付与して、それをkeyにするのがベストな気もする。ので、一旦、実際はバックエンド側で付与するIDのモックを仮置きの商品データに追加して、それをkeyにしてもらえる?

Copy link
Contributor

Choose a reason for hiding this comment

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

content.product.namekeyにするのは避けるべきかも。

これっぽい。
スクリーンショット 2023-04-06 7 25 36

pages/[shopId]/order.tsx Outdated Show resolved Hide resolved
pages/[shopId]/order.tsx Show resolved Hide resolved
{Object.values(OrderContents).map((content) => {
return (
<li className="relative mb-[16px] pt-[18px] pb-[15px] pr-[14px] pl-[11px] w-full h-[94px] bg-navajowhite-a100 rounded-[13px]">
<div className="grid grid-cols-5 grid-rows-1">
Copy link
Contributor

Choose a reason for hiding this comment

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

これgrid使う必要あるかな、、
デザイン的に、あんまり法則性のある配置には見えないからgridを使う必要があるようには思えなくて、もし特にgridを使う必要がなければ、gridを使わない書き方に変えてほしい!

Copy link
Contributor

@umiteru2004 umiteru2004 left a comment

Choose a reason for hiding this comment

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

よろしくお願いします!

/>
</div>
<div className="absolute top-[176px] mb-[50px] pt-[32px] pr-[15px] pl-[16px] w-full h-full bg-white-a100 rounded-t-[16px]">
<p className="mb-[16px] text-brown-a100 text-[20px] font-bold">
Copy link
Contributor

Choose a reason for hiding this comment

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

h1使って〜

<ul>
{Object.values(OrderContents).map((content) => {
return (
<li
Copy link
Contributor

Choose a reason for hiding this comment

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

ちょっと商品カードのスタイルがごちゃってるかも。
もう1回スタイルの当て方を見直して、よりシンプルで、かつデザインに忠実なスタイリングを極めて見てください!
参考までに、俺だったらどうするかを。俺だったら赤わくのflexに青枠3つを並べて、ADDボタンだけposition: absoluteするかも。
スクリーンショット 2023-03-11 7 11 12

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