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

Adds Vehicle support and images for views #53

Closed
wants to merge 9 commits into from
Closed

Adds Vehicle support and images for views #53

wants to merge 9 commits into from

Conversation

niamhdurfee
Copy link

@niamhdurfee niamhdurfee commented Oct 23, 2016

Fixes issues #51 and #7

@@ -26,3 +27,33 @@ starship.fetch = id => {
}, reject);
});
};
starship.getPicture = searchWord => {
Copy link
Owner

Choose a reason for hiding this comment

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

This picture search appears to be the exact same for each of the component API files. Would prefer moving this to a picture API file and delegating to that.

});
},
},
};
</script>

<!-- Add "scoped" attribute to limit CSS to this component only -->
<style scoped>
Copy link
Owner

Choose a reason for hiding this comment

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

This block is copied across the components. Would be a good candidate to push to a style file that can be imported into components that need it.

@alexkramer
Copy link
Owner

Looks pretty good! If possible see if you can add a small test of the new functionality you tested. I also realize that the tests are failing across the builds right now due to Firefox. Do not worry about that, I will take a look and see if I can fix that.

@niamhdurfee
Copy link
Author

Still looking into writing tests for functionality for #7, fixed issues from before for #51.
The tests are failing on Chrome for me, but FireFox works fine?!

@alexkramer alexkramer closed this Mar 11, 2017
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