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

Add photo display feature and routing #20

Merged
merged 6 commits into from
Jun 2, 2022
Merged

Conversation

rtomitani
Copy link
Contributor

@rtomitani rtomitani commented May 30, 2022

  • パスに/p/:photoIdを指定すると、Firestoreのphotos/photoIdの写真を地図上に表示する
  • Firestoreセキュリティルールを変更し、photosコレクションを誰でもread可能にした

image

@rtomitani rtomitani closed this May 30, 2022
@github-actions
Copy link

github-actions bot commented May 30, 2022

Visit the preview URL for this PR (updated for commit 79cb3f6):

https://nounsmap-web-dev--pr20-feature-photo-displa-u0gihanq.web.app

(expires Wed, 08 Jun 2022 23:57:12 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@rtomitani rtomitani reopened this May 30, 2022
Copy link
Collaborator

@kozayupapa kozayupapa left a comment

Choose a reason for hiding this comment

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

@rtomitani 素晴らしいです ありがとうございます。
直前にマージしてしまいコンフリクトしてしまっている箇所があるので対応お願いします。

src/router/index.ts Outdated Show resolved Hide resolved
src/router/index.ts Outdated Show resolved Hide resolved
src/components/NounsMap.vue Outdated Show resolved Hide resolved
src/components/NounsMap.vue Outdated Show resolved Hide resolved
@rtomitani
Copy link
Contributor Author

@kozayupapa @isamu
ご指摘いただいた内容全て修正できました
ご確認いただければと思います〜

lng,
visibility: true
});
if (!isNaN(lat) && !isNaN(lng)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lat, lngは、firestoreからとってきているので、NaNではなく、undefinedになりそうです。
undefined or nullが、よくあるケースなので、その判定をする関数を作って判断するのが良いと思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます!
zoomプロパティ含め、undefined or nullの判定を行うよう修正します

後学のためにお聞きしたいのですが、プロパティに数値とnull / undefined以外の値(例えばArrayなど)が入っている可能性を考慮した場合、どのようにチェックしたら良いですか?
また、そもそもチェックの必要性はありますか?
以前Firestoreを使ったときにこの辺りのチェックがよくわからず、なあなあにしていました

Copy link
Collaborator

@kozayupapa kozayupapa left a comment

Choose a reason for hiding this comment

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

ありがとうございます!

@isamu
Copy link
Contributor

isamu commented Jun 1, 2022

ありがとうございます!
isNaN以外は問題なさそうなので、このままマージするか、もしくは、isNaNだけ修正してマージをお願いします。

@rtomitani rtomitani merged commit 53a849c into main Jun 2, 2022
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.

None yet

3 participants