-
Notifications
You must be signed in to change notification settings - Fork 1
物品返却時の画面作成 #64
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
base: main
Are you sure you want to change the base?
物品返却時の画面作成 #64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いくつかコメントしましたー
public/550 1.jpg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
名前に空白入るの良くなさそう & もっと分かりやすい名前にすると良さそう & jpg どこでも使ってなさそう
src/router/index.ts
Outdated
@@ -1,6 +1,8 @@ | |||
import type { RouteRecordRaw } from 'vue-router'; | |||
import { createRouter, createWebHistory } from 'vue-router'; | |||
import registerView from '@/pages/registerView.vue'; | |||
import returnCheck from '@/pages/ReturnCheck.vue'; | |||
import returnCheckok from '@/pages/ReturnCheckOK.vue'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コンポーネントはパスカルケースの方が良いかも (ReturnCheckOK
みたいな)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これ他のも統一しちゃうね
src/pages/ReturnCheckOK.vue
Outdated
|
||
<script lang="ts" setup> | ||
import MainHeader from '@/shared/lib/components/MainHeader.vue'; | ||
import returnSvg from '/mdi_check-circle.svg'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import するなら public フォルダではなく src/assets 配下に入れておくのが良いかも
@@ -15,6 +17,16 @@ const routes: RouteRecordRaw[] = [ | |||
name: 'register', | |||
component: registerView, | |||
}, | |||
{ | |||
path: '/return-check', | |||
name: 'return-check', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
パスに物品IDとかは含まないのかな?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
含むべきではある、けどAPI完成してないから単に加えていないだけだね
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
最初からAPI完成してる前提のルーティングしておいた方が良さそう
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
productTitle
は衝突しないことが保証されてるのかな?
<template> | ||
<div><MainHeader></MainHeader></div> | ||
<div :class="$style['svgContainer']"> | ||
<img :src="returnSvg" alt="QR Code" :class="$style['svgImage']" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alt="QR Code"
はどういう意図なんだろう? たぶん alt=""
にしておくのが良いと思います
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ、alt(空白)可能なのを知らんかったです
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alt はその画像をそのテキストに置換したときに意味が伝わるものにすることを意識して決めると良いです👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(逆に装飾目的の画像なら alt=""
で良い)
src/pages/ReturnCheck.vue
Outdated
|
||
// APIが完成するまでのダミーデータ | ||
const userName = 'o_ER4'; | ||
const bookTitle = 'Vue.js入門'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
book ではなく product の方が良いかな? 本以外にも物品はあるので
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かに
src/pages/ReturnCheck.vue
Outdated
<ChipCard :label="label1" /> | ||
<ChipCard :label="label2" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここでラベルが変数化されてるのはどういう意図があるんだろう? (ここにどういう文言が入り得る?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なんかAPI値習得するかとか頭バグって考えていそう(今見れば深い意味ないのでlabelは直書きしちゃいます)
おきもち程度にAPIとの接合部分書きました |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API周りについてちょっとコメントしましたー
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return-icon
みたいな感じでケバブケースにしても良いかも (もう1つのアイコンも同じく)
<strong v-if="userName">{{ userName }}</strong> | ||
<span v-else>ユーザー情報</span> さんに「<strong v-if="productTitle">{{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
userName === ''
の時の挙動は「ユーザー情報」と表示されるのはユーザーにとってはあんまり嬉しくなさそう? userName === ''
はAPIから値を取得中のときだと思うんだけど、その時にどういう表示になってるとユーザーが嬉しいかを考えてみると良さそう
他の部分も同じく
以下いくつかアイデア それぞれのメリットデメリットは自分で考えてみてください
- 取得中は画面全体を表示しない
- 一瞬白い画面を許容する
- 取得中はこのメッセージを表示しない
- 画面に後からポッと出てくる感じ
- メッセージごとに個別に出てくるのか、一緒に出てくるのかはまた考える必要がある
- メッセージにスケルトンを入れる
- スケルトン = この部分が読み込み中ですよー、というのをユーザーに示すためのもの
- 今はこの実装っぽくなってそうだけど、「ユーザー情報」という文字列を見てユーザーはスケルトンと認識できなさそう...?
@@ -15,6 +17,16 @@ const routes: RouteRecordRaw[] = [ | |||
name: 'register', | |||
component: registerView, | |||
}, | |||
{ | |||
path: '/return-check', | |||
name: 'return-check', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
productTitle
は衝突しないことが保証されてるのかな?
path: '/return-check/ok', | ||
name: 'return-check-ok', | ||
component: ReturnCheckOk, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/return-check
と同じくだけど、こっちは必ず /return-check/ok
に返ってくるという実装でも良さそう (ユーザーごとのデータとかを表示しないなら)
<div :class="$style['svgContainer']"> | ||
<img :src="returnSvg" alt="QR Code" :class="$style['svgImage']" /> | ||
</div> | ||
<div>返却されました</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
デザインにないことを注文して申し訳ないんだけど、もし良かったら物品詳細への動線とかを置いておくと良さそう
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
らじゃ、作ったらfigmaのデザインにも反映させときます
あと多分その手の変更がほかにも生じそう
今ありそうなのはreturncheckの返却における処理失敗の表示とか
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API周りについてちょっとコメントしましたー
やったこと
やってないこと