textInput を実装 #61
textInput を実装 #61
Conversation
cp-20
left a comment
cp-20
left a comment
There was a problem hiding this comment.
細々としたところだけど、特にアクセシビリティ的なところは修正して欲しいです！
cp-20
left a comment
cp-20
left a comment
There was a problem hiding this comment.
細々としたところだけど、特にアクセシビリティ的なところは修正して欲しいです！
cp-20
left a comment
cp-20
left a comment
There was a problem hiding this comment.
色々レビュー漏れがあったのでコメントしました:bow::bow::bow:
src/shared/components/TextInput.vue
Outdated
| import { defineProps } from 'vue'; | ||
|
|
||
| defineProps<{ | ||
| placeholder: string; |
There was a problem hiding this comment.
placeholder は optional にしてほしい！
src/shared/components/TextInput.vue
Outdated
|
|
||
| defineProps<{ | ||
| placeholder: string; | ||
| size: number; |
There was a problem hiding this comment.
size というのは若干直感的ではないかも？ (HTML要素の <input> に直接渡してるけど、これは求めているものではなさそう)
雑にやるなら size はなくて良くて、ちゃんとやりたいなら 'sm' | 'md' | 'lg' みたいな感じでやると良さそうに見えます！
src/shared/components/TextInput.vue
Outdated
| <div v-if="$slots['left']" class="side-icon"> | ||
| <slot name="left" /> | ||
| </div> |
There was a problem hiding this comment.
[optional] <slot> に直接 class を書いた方がスッキリするかも
| border-radius: 4px; | ||
| } | ||
| .background:focus-within { | ||
| outline: 2px solid; |
There was a problem hiding this comment.
outline は primary-color とかの方が良い感じかも
src/shared/components/TextInput.vue
Outdated
| border-color: $color-border; | ||
| border: 1px solid; |
There was a problem hiding this comment.
border-color プロパティが border ショートハンドによって上書きされてそう (ref: https://developer.mozilla.org/ja/docs/Web/CSS/border#%E6%A7%8B%E6%88%90%E8%A6%81%E7%B4%A0%E3%81%AE%E3%83%97%E3%83%AD%E3%83%91%E3%83%86%E3%82%A3)
border で一緒に指定してあげると良さそう！
|
直したら review request もらえると嬉しいです:bow: |
src/shared/components/TextInput.vue
Outdated
| display: inline flex; | ||
| height: 32px; | ||
| align-items: center; | ||
| border: |
There was a problem hiding this comment.
1px solid $color-border で良さそうな気がする！ radius はここでは指定できないので別で指定してあげる必要がある
src/shared/components/TextInput.vue
Outdated
| } | ||
| .text-input::placeholder { | ||
| color: $color-text-placeholder; | ||
| opacity: 1; |
src/shared/components/TextInput.vue
Outdated
| height: 32px; | ||
| align-items: center; | ||
| border: 1px solid1px solid $color-border; | ||
| border-radius: 1px solid; |
There was a problem hiding this comment.
border-radius は solid を受け付けない (それは border-style) し、ここで指定すべきは 4px じゃないかな？
Button 要素ごと template で持ってくることを想定しています
入力欄とそのほかの区別がわかりづらいことがちょっと気になってるんですが、こういうもんなんでしょうか
close: #51