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

Audrey's final MediaRanker #28

Open
wants to merge 45 commits into
base: ald/master
Choose a base branch
from
Open

Audrey's final MediaRanker #28

wants to merge 45 commits into from

Conversation

Dreedle
Copy link

@Dreedle Dreedle commented Dec 4, 2015

Did accomplish:
-The requirements for waves 1, 2 and 3.
-98% code coverage on tests. (The model tests do not have complete coverage because I did not write a test for my method to order the items by ranking. I figured that method is entirely ActiveRecord, and I only wrote it to make the code in the views more concise.)
-Using shared examples to do rspec tests.

Did not accomplish:
-Drying out views/controllers with polymorphic path. Once I had written the tests, I decided that ship had sailed.
-On index page for books, albums and movie, the spacing of table rows look slightly different. Chalking it up to dynamicity of the table rows, but not entirely sure. Couldn't find a css difference.

…top ten yet). Did put in some bootstrap classes in anticipation of the future
…s (bootstrap still has not been added so they don't look like anything yet)
@@ -0,0 +1,2 @@
--color
Copy link

Choose a reason for hiding this comment

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

It's not much of an issue to have this included in the project repo, although you could have put this .rspec file in your home directory and it would work for your local machine (across all projects).

@Hamled
Copy link

Hamled commented Dec 7, 2015

Excellent use of many small, focused commits!
Also, while I agree that it's probably too much to DRY out the controllers and models by moving everything to a single "Medium" model, I think it wouldn't be too far from what you've currently got to DRY out some of the views by combining the edit and create forms into a single generic form partial.

Overall it's great, keep it up!

@Hamled Hamled self-assigned this Dec 7, 2015
@Dreedle
Copy link
Author

Dreedle commented Jan 2, 2016

The comments about specific ways to do testing were really helpful! Thanks

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