Skip to content

Conversation

@joshsmith
Copy link
Contributor

This adds the beginning of navigation. No tests, so there's lots to be done here. Could use plenty of help.

@joshsmith
Copy link
Contributor Author

screen shot 2016-02-15 at 8 40 53 pm

@begedin
Copy link
Contributor

begedin commented Feb 17, 2016

To do

  • Integration tests for user-menu under integration/components/user-menu.js
  • Acceptance tests for home page
    • When logged out
    • When logged in
      • User menu visible
      • User menu links work

We need acceptance tests because integration tests seem to not have access to the router, so the links don't have the proper URLs.

Fix issues with reliance on typeKey instead of modelType/ownerType

Mirage does not create actual model instances. Instead, it creates instances of its own model class. This means there is no typeKey, which is why I was using a manually set modelType property, which would now, I guess, become an ownerType property. The API already has this property, due to the nature of the polymorphic relationship, so it was just a matter of adding it to the serializer. This is what Mirage was relying on up to this point.

Additionally, due to the same reason, Mirage can't handle computed properties defined on the model. They don't get transferred over to the mirage model. For that reason, I used computed properties on the component level.

In order to resolve this we need to

  • Replace all referrences to modelType with ownerType
  • Revert back to using computed properties on the component level instead of model level

I'm afraid it's not a nice solution, but it's the only one I can think off.

joshsmith added a commit that referenced this pull request Feb 18, 2016
@joshsmith joshsmith merged commit 9b37025 into develop Feb 18, 2016
@joshsmith joshsmith deleted the add-navigation branch February 18, 2016 06:37
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