Skip to content
This repository has been archived by the owner on Sep 8, 2020. It is now read-only.

Fixing issues #3, #9, #11 #17

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
21856d0
#9 - Added route to deal with non-matching path and a new component f…
sfrankian May 27, 2018
26008e5
#11 Fixed people request to work properly and added a redirect if a '…
sfrankian May 27, 2018
0161211
Merge remote-tracking branch 'upstream/master'
sfrankian May 27, 2018
0e8f56c
Added vue-notification to package.json which makes the error notifica…
sfrankian May 27, 2018
db1d933
Changed title of profile page from 'Person n' to name retrived from A…
sfrankian May 27, 2018
86154cb
#3 added person profile page. Still need to add links to related reso…
sfrankian May 28, 2018
c732690
Fixed error in function that fetches person details from the API
sfrankian May 28, 2018
9c96db7
#3 changed rendering of person component to only show vehicles if per…
sfrankian May 28, 2018
a9de522
Simplifying API get request
sfrankian May 28, 2018
0c2314a
#3 Finished (just need to write tests). Because planet/starship compo…
sfrankian May 28, 2018
b980b0c
#3 - forgot to make homeworld a link.
sfrankian May 28, 2018
cf8ee4f
Made person template conditionally render based on whether we have re…
sfrankian May 28, 2018
9d55696
Removed console.logs and added a few comments
sfrankian May 28, 2018
99133ed
Added another cypress test and created a file to run unit tests on th…
sfrankian May 28, 2018
5abaca4
Fixed test description name to be Person, not home
sfrankian May 30, 2018
0402e4a
Fixed person tests to check for correct rendering
sfrankian May 30, 2018
1086e79
Adding package.json and package-lock.json so that CI can pass
sfrankian May 30, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions cypress/integration/home_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,20 @@ describe('Home', () => {
cy.visit('/')
})
})
describe('PageNotFound', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: space above.

it('should load error page', () => {
cy.visit('/#/thisisnotavalidpath')
})
})

describe('People', () => {
it('should load', () => {
cy.visit('/#/people/1')
})
})

describe('People', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

With BDD style you'd want to have unique describe blocks containing multiple it blocks if necessary. The it block below could join the other it block above.

it('should handle bad person request', () => {
cy.visit('/#/people/41234')
})
})
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"dependencies": {
"unfetch": "^3.0.0",
"vue": "^2.5.16",
"vue-notification": "^1.3.7",
"vue-router": "^3.0.1",
"vuex": "^3.0.1"
},
Expand Down
1 change: 1 addition & 0 deletions src/components/App.vue
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<template>
<div>
<main>
<notifications group="notifs" />
<router-view />
</main>
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/components/Home.vue
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
<div class="d-flex flex-wrap">
<router-link
v-for="(person, id) in results"
:to="`/people/${id}`"
:to="`/people/${id + 1}`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

:key="id"
class="card m-2"
style="width: 12rem;">
Expand Down
14 changes: 14 additions & 0 deletions src/components/PageNotFound.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<template>
<div class="container-fluid">
<h2>Uh oh, the force was not with that request.</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great!

<img
:src="'https://upload.wikimedia.org/wikipedia/commons/thumb/1/1e/YODA_%283343464749%29.jpg/640px-YODA_%283343464749%29.jpg'"
alt="Yoda image">
<div class="row pt-3 pl-3">
<router-link
:to="`/`">
<button>Return home</button>
</router-link>
</div>
</div>
</template>
71 changes: 62 additions & 9 deletions src/components/Person.vue
Original file line number Diff line number Diff line change
@@ -1,25 +1,78 @@
<template>
<div>
Person {{ $route.params.id }}
<div class="container">
<!--Only renders template if given results.-->
<div class="card p-3" v-if="results !== undefined || results.length !== 0">
<h1>{{results.name}}</h1>
<h4>Biography</h4>
<ul class="list-group pb-3" id="bio" >
<li class="list-group-item">Homeworld: <a :href="results.homeworld.url">{{ results.homeworld.name }}</a></li>
<li class="list-group-item">Birth year: {{ results.birth_year }}</li>
<li class="list-group-item">Height: {{ results.height }}</li>
<li class="list-group-item">Mass: {{ results.mass }}</li>
<li class="list-group-item">Species: {{ results.species }}</li>
<li class="list-group-item">Hair color: {{ results.hair_color }}</li>
<li class="list-group-item">Eye color: {{ results.eye_color }}</li>
<li class="list-group-item">Skin color: {{ results.skin_color }}</li>
</ul>
<ul class="list-group pb-3">
<h4 v-if="results.starships===undefined || results.starships.length !== 0">Starships</h4> <!-- Only show starship if person has starships -->
<li v-for="starship in results.starships" class="list-group-item">
<a :href="starship.url">{{ starship.name }}</a>
</li>
</ul>
<ul class="list-group">
<h4 v-if="results.vehicles===undefined || results.vehicles.length !== 0">Vehicles</h4> <!-- Only show vehicles if person has vehicles -->
<li v-for="vehicle in results.vehicles" class="list-group-item">
<a :href="vehicle.url">{{ vehicle.name }}</a>
</li>
</ul>
<div class="row pt-3 pl-3">
<router-link
:to="`/`">
<button>Return home</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip: you can make router-link into a button with the as property.

</router-link>
</div>
</div>
<div class="row" v-else>
<p>uh oh nothing to show here</p>
</div>
</div>
</template>

<script>
import { mapGetters } from 'vuex'
import { mapGetters, mapActions, mapState } from 'vuex'

import store from '@/store'

export default {
async beforeRouteEnter (to, from, next) {
await store.dispatch('people/get', to.params.id)
next()
const resp = await store.dispatch('people/getPerson', to.params.id)
// If we receive null back as a response, we redirect back to the home page
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: A better way to handle this would have been with a try-catch block.

// due to the invalid request
if(resp === null) {
next('/')
} else {
next()
}
},
async beforeRouteUpdate (to, from, next) {
await store.dispatch('people/get', to.params.id)
next()
const resp = await store.dispatch('people/getPerson', to.params.id)
// Sends a quick notification that the request failed and
// then redirects back to the home page
if(resp === null) {
this.$notify({
group: 'notifs',
title: 'Error: ' + 'invalid path for a person',
text: 'Taking you back home.',
type: 'warn'
});
next('/')
} else {
next()
}
},
computed: {
...mapGetters('people', ['person'])
}
...mapState('people', ['results'])
},
}
</script>
4 changes: 4 additions & 0 deletions src/main.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Vue from 'vue'
import Notifications from 'vue-notification'

import router from '@/router'
import store from '@/store'
Expand All @@ -8,5 +9,8 @@ import App from '@/components/App'
// Create the Vue application instance.
const app = new Vue({ router, store, render: h => h(App) })

// Adding for notifications
Vue.use(Notifications)

// Mount the application to the element on the page with id="app".
app.$mount('#app')
6 changes: 5 additions & 1 deletion src/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Vue.use(VueRouter)

const Home = () => import('@/components/Home')
const Person = () => import('@/components/Person')

const PageNotFound = () => import('@/components/PageNotFound')
// Export a new Vue Router instance to be used in the application.
export default new VueRouter({
routes: [
Expand All @@ -17,6 +17,10 @@ export default new VueRouter({
{
path: '/people/:id',
component: Vue.component('Person', Person)
},
{
path: '*',
component: Vue.component('PageNotFound', PageNotFound)
}
]
})
82 changes: 72 additions & 10 deletions src/store/people.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,18 @@ const initialState = () => ({
results: []
})

// Helper function to fetch links from the API and return name
// of starship/planet/vehicle/etc.
async function renderAttributeName (url) {
const results = await fetch(url)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing because you're mixing 2 different but interchangeable concepts: promises and async/await.

.then(function (response) {
if (response.ok) {
return response.json()
}
})
return results.name
}

// Export the module so it can be included in the main store.
export default {
// This module is namespaced so that it is a little more self-contained:
Expand All @@ -17,17 +29,67 @@ export default {
actions: {
async list ({ commit }, page = 1) {
// Fetch People from SWAPI and await the response.
const response = await fetch('https://swapi.co/api/people')
// Parse the JSON string returned in the response into a JavaScript
// object and destructure the results property out of that object.
const { results } = await response.json()
if (response.ok) {
// If the response is OK commit the data to the store.
commit('results', results)
} else {
// If the response is not OK, log the response to the console.
console.error(response)
const results = await fetch('https://swapi.co/api/people')
.then(function (response) {
if (response.ok) {
return response.json()
}
})
commit('results', results.results)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this change is a step back because it's not handling the error case.

},
async getPerson ({ commit }, id) {
// Fetch People from SWAPI and await the response.
const apiUrl = 'https://swapi.co/api/people/' + id

const results = await fetch(apiUrl)
.then(function (response) {
if (response.ok) {
return response.json()
} else {
return null
Copy link
Contributor

Choose a reason for hiding this comment

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

You generally don't want to be returning values in store actions. The data should flow in a single direction so that the store is the single source of truth for consumers.

}
})
if (results === null) {
return null
}
const homeworldUrl = results.homeworld
const homeworld = await renderAttributeName(homeworldUrl)

// saving in this format for easy template rendering
results.homeworld = { 'name': homeworld, 'url': homeworldUrl }

const species = await renderAttributeName(results.species[0])
results.species = species

// Only fetch the vehicle names from the API if the person has vehicles
if (results.vehicles !== undefined || results.vehicles.length !== 0) {
let vehicles = []
for (var i = 0; i < results.vehicles.length; i++) {
var vehicleUrl = results.vehicles[i]
const vehicleName = await renderAttributeName(vehicleUrl)
vehicles[i] = {'name': vehicleName, 'url': vehicleUrl}
}
results.vehicles = vehicles
}

// Only fetch the starship names from the API if the person has starships
if (results.starships !== undefined || results.starships.length !== 0) {
let starships = []
for (var j = 0; j < results.starships.length; j++) {
var starshipUrl = results.starships[j]
const starshipName = await renderAttributeName(starshipUrl)
starships[j] = {'name': starshipName, 'url': starshipUrl}
}
results.starships = starships
}

commit('results', results)
return results
}
},
getters: {
getResults: state => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a useful getter since it's only returning the state which can already be retrieved by other methods.

return state.results
}
}
}
17 changes: 17 additions & 0 deletions tests/person.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { mount, createLocalVue } from '@vue/test-utils'
import Vuex from 'vuex'

import store from '@/store'
import Person from '@/components/Person'
import 'isomorphic-fetch'
import VueRouter from 'vue-router'

const localVue = createLocalVue()

localVue.use(Vuex)
localVue.use(VueRouter)

test('Home renders correctly', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test description should probably say Person instead of Home.

const wrapper = mount(Person, { store, localVue })
expect(wrapper.html()).toMatchSnapshot()
})