Fixing issues #3, #9, #11 #17
Conversation
…omponent for page not found.
…ect if a 'people' API request didn't return OK.
…tions for issue binxhealth#9 much prettier.
…lated resources
…les if person has them
…ship components are not available, just linking to API for now.
…sults in the datastore (binxhealth#3). Also added back in error handling for incorrect people path - relates to binxhealth#9.
…e person component
|
|
|
Oops, looks like I didn't configure the continuous integration service to test forks. I'm going to leave some minor feedback on your changes. Would you mind addressing this feedback and pushing a new commit? That should trigger the CI build. |
tests/person.test.js
Outdated
| localVue.use(Vuex) | ||
| localVue.use(VueRouter) | ||
|
|
||
| test('Home renders correctly', () => { |
There was a problem hiding this comment.
This test description should probably say Person instead of Home.
| @@ -3,3 +3,20 @@ describe('Home', () => { | |||
| cy.visit('/') | |||
| }) | |||
| }) | |||
| describe('PageNotFound', () => { | |||
| }) | ||
| }) | ||
|
|
||
| describe('People', () => { |
There was a problem hiding this comment.
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.
| @@ -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}`" | |||
| <div class="row pt-3 pl-3"> | ||
| <router-link | ||
| :to="`/`"> | ||
| <button>Return home</button> |
There was a problem hiding this comment.
Tip: you can make router-link into a button with the as property.
| 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 |
There was a problem hiding this comment.
FYI: A better way to handle this would have been with a try-catch block.
| if (response.ok) { | ||
| return response.json() | ||
| } else { | ||
| return null |
There was a problem hiding this comment.
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.
| } | ||
| }, | ||
| getters: { | ||
| getResults: state => { |
There was a problem hiding this comment.
This isn't a useful getter since it's only returning the state which can already be retrieved by other methods.
| localVue.use(Vuex) | ||
|
|
||
| const results = { | ||
| 'name': 'Darth Vader', |
There was a problem hiding this comment.
FYI: quotes not necessary for object keys.
| 'results': results | ||
| } | ||
|
|
||
| var store = new Vuex.Store({ |
There was a problem hiding this comment.
Not sure why we're creating a new store here instead of just importing the actual store like in home.test.js.
| @@ -0,0 +1,14 @@ | |||
| <template> | |||
| <div class="container-fluid"> | |||
| <h2>Uh oh, the force was not with that request.</h2> | |||
So far I've done issues #3, #9, and #11, but will open another pull request for future issue fixes.
Also, thanks for the fun assignment!