Skip to content

Jason challenge#3

Open
Eventlessdrop wants to merge 4 commits intokayac:masterfrom
Eventlessdrop:jason-challenge
Open

Jason challenge#3
Eventlessdrop wants to merge 4 commits intokayac:masterfrom
Eventlessdrop:jason-challenge

Conversation

@Eventlessdrop
Copy link
Copy Markdown

申し訳ありませんどうなったとわかんないけどこれ大丈夫ですか?

I would like to sincerely apologize for the inconvenience caused, I was not aware that the pull request had closed

Comment thread pages/Answers/index.vue
Comment on lines +39 to +41
this.answer1 = this.$route.params.answer1
this.answer2 = this.$route.params.answer2
this.answer3 = this.$route.params.answer3
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

answer は、vuexのstateで管理すると良いかと思います

nuxt のドキュメントを貼っておきます
https://nuxtjs.org/guide/vuex-store

Comment thread pages/Answers/index.vue Outdated
Comment on lines +51 to +58
if (this.potentialResult[i].result1 === this.answer1) {
if (this.potentialResult[i].result2 === this.answer2) {
if (this.potentialResult[i].result3 === this.answer3) {
this.final = this.potentialResult[i]
}
}
}
}
Copy link
Copy Markdown
Contributor

@ko-hioki ko-hioki Nov 8, 2019

Choose a reason for hiding this comment

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

if文の階層が深くなり、わかりにくくなっているので下記のよう書くこともできます

if (this.potentialResult[i].result1 !== this.answer1) continue
if (this.potentialResult[i].result2 !== this.answer2) continue
if (this.potentialResult[i].result3 !== this.answer3) continue

this.final = this.potentialResult[i]
break

Comment thread pages/Answers/index.vue
// this.openGraph()
},
methods: {
determineResult() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

判定のロジックは stateのActionで判定させて
pages/Answers/index.vue では結果のみを表示させたいですね

Comment thread pages/Answers/index.vue
}
}
},
sharableresult() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ここのシェアの判定もロジックは stateのActionで判定させて
pages/Answers/index.vue では結果のみを表示させたいですね

Comment thread pages/Answers/index.vue Outdated
.city-image{
max-width: 1000px;
max-height: 600px;
/*background-image: url('~assets/kamakura.jpg');*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

こちらのコメントは今後使用しますでしょうか?

使用していないコメントなどはレビュー前に消しておくと良いですよ

Comment thread pages/Questions/One.vue Outdated
</h1>
<div class="button-container">
<button class="quiz-button">
<nuxt-link class="button-label" :to="{name: 'Questions-Two', params: { answer1: 'Yes' }}">
Copy link
Copy Markdown
Contributor

@ko-hioki ko-hioki Nov 8, 2019

Choose a reason for hiding this comment

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

質問の回答はパラメーターを渡すのではなく、vuexのstateに入れて管理してみましょう

Comment thread pages/index.vue
Comment on lines +45 to +47
margin-right: 43vw;
margin-left: 5vw;
margin-bottom: 30vh;
Copy link
Copy Markdown
Contributor

@ko-hioki ko-hioki Nov 8, 2019

Choose a reason for hiding this comment

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

margin: auto 5vw 30vh 43vw; と書けます
細かな指定は vh や vwではなく pxなどで指定したほうが良さそうですね

また表示位置などでしたら、Positionでも指定可能です

Comment thread pages/index.vue
Comment on lines +71 to +72
margin-left: 5vh;
margin-right: 5vh;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

margin: auto 5vh; と書けます

Comment thread components/Header.vue Outdated
@@ -0,0 +1,46 @@
<template>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

こちらのファイル(components/Header.vue)は今回の課題で使用していますか?

Comment thread pages/Answers/index.vue
if (this.potentialResult[i].result3 !== this.answer3) continue

this.final = this.potentialResult[i]
break
Copy link
Copy Markdown
Contributor

@ko-hioki ko-hioki Nov 25, 2019

Choose a reason for hiding this comment

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

コードがスッキリわかりやすくなったと思います 👍

id: this.$route.params.id
}
},
computed: {
Copy link
Copy Markdown
Contributor

@ko-hioki ko-hioki Nov 25, 2019

Choose a reason for hiding this comment

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

ストアの値を呼び出すときは
vuex で mapStateなどのHelperが用意されているのでこちらを使うと良いでしょう
https://vuex.vuejs.org/guide/state.html


export default {
components: {
Question: Question
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Comment thread store/index.js
export const getters = {}
export const mutations = {
increment(state) {
if (state.count < 4) {
Copy link
Copy Markdown
Contributor

@ko-hioki ko-hioki Nov 25, 2019

Choose a reason for hiding this comment

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

mutations はstateに値を設定だけの役割なので、分岐などはActionsでチェックを行い、commitする使用し行うと良いかと思います

Comment thread store/index.js
state.count++
}
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

今回Storeで管理したいものは下記のものなので、
store/answers.js と store/questions.js は一つにまとまっている方が扱いやすいと思います

  • 回答の進捗
  • 回答結果

Comment thread store/questions.js
const correctAnswers = answers.filter(answer => answer)
return correctAnswers / state.questions.length
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

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.

2 participants