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

検索フォームの実装 #43

Merged
merged 4 commits into from
Oct 27, 2022
Merged

検索フォームの実装 #43

merged 4 commits into from
Oct 27, 2022

Conversation

mehm8128
Copy link
Contributor

inputformと似たような感じになっちゃいましたけど大丈夫ですかねこれ

@tesso57
Copy link
Member

tesso57 commented Oct 19, 2022

https://icon-sets.iconify.design/mdi/at/ @のアイコンがあるので、hasAtMark の部分を atmagnify しか受け取れない型に変えて、実装してもよさそう。

@mehm8128
Copy link
Contributor Author

https://icon-sets.iconify.design/mdi/at/ @のアイコンがあるので、hasAtMark の部分を atmagnify しか受け取れない型に変えて、実装してもよさそう。

なるほど、よさそうです!
やってみたのですが、FormSearchもコンポーネントごと消しちゃって使うときにiconを指定する感じでよさそうですかね?

@tesso57
Copy link
Member

tesso57 commented Oct 19, 2022

そうですね。今あるやつはなくして、FormInputを改修する感じですね。

@mehm8128 mehm8128 requested review from sapphi-red, toshi-pono and tesso57 and removed request for sapphi-red October 20, 2022 14:27
@@ -40,7 +40,9 @@ const handleInput = (event: Event) => {

<template>
<div :class="$style.container" :data-has-anchor="props.hasAnchor">
<span v-if="props.hasAtmark" :class="$style.atmark"> @ </span>
<div v-if="props.icon !== undefined" :class="$style.iconContainer">
Copy link
Member

Choose a reason for hiding this comment

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

めっちゃ細かいんですが、typeof icon !== 'undefined' の方がいいかもしれません。

参考: https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

strict modeではundefinedを上書きできないので元のままでも大丈夫ですね 🖖 ESMは常にstrict mode)

下を実行するとエラーになります

function f () {
    "use strict";
    undefined = 'po';
}
f();

Copy link
Member

Choose a reason for hiding this comment

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

ありがとうございます 🙇‍♂️🙇‍♂️🙇‍♂️

Copy link
Contributor

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

:yosa: :desu:

@mehm8128 mehm8128 merged commit 808e062 into main Oct 27, 2022
@mehm8128 mehm8128 deleted the feat/search-form branch October 27, 2022 06:32
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