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

Final Version of MediaRanker #30

Open
wants to merge 77 commits into
base: mmr/master
Choose a base branch
from
Open

Conversation

knaydee
Copy link

@knaydee knaydee commented Dec 4, 2015

Complete thru wave 3 and includes medium_spec.rb and shared examples. Trie but couldn't get media_controller_specs to work.

…stall gem, add movie show view with html and ruby code, create movie edit file
@Hamled Hamled self-assigned this Dec 9, 2015
@Hamled Hamled removed their assignment Jan 5, 2016
@QuartzBrandi QuartzBrandi self-assigned this Jan 6, 2016

def new
@album = Album.new
@title = "Add New album"
Copy link

Choose a reason for hiding this comment

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

Nice job prompting the user on what to do here! To prompt the user even further, you could use the placeholder attribute on form fields in the view to give some example form input.

@QuartzBrandi QuartzBrandi removed their assignment Jan 14, 2016
</div>
<div class="row">
<div class="col-lg-12">
<body>
Copy link

Choose a reason for hiding this comment

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

This is confusing. There are several display elements outside the body in this file. Although they may display properly in some modern browsers, I suspect this ordering would wreak havoc on older browsers and might cause issues with interpreting the DOM or otherwise prevent your site from displaying as you intend to said users. I suspect that what you're looking for here is main, which is a semantic tag used to denote which part of the DOM contains the main content of a page.

@sojeri
Copy link

sojeri commented Jan 14, 2016

You did an excellent job emulating the style of the original Media Ranker project, and I enjoyed your attempt at DRYing up those tests by using shared examples -- even if the media controller examples didn't work out. You exposed some data to the view layer that I felt was unnecessary and had a couple holes in your update controller actions, so that's a couple things to look out for in future projects.

When writing tests, don't forget to test the non-required fields, too! That's a great way to check whether an update action is working right. I hope you've had a chance to start experimenting with controller filters, since you already started using the core of the idea with your get_medium methods in this project!

This is a pretty solid project! Great job, @knaydee.

If you have any questions or want me to poke a little harder at part of the code, feel free to reach out to me. Be sure to @mention me in that case to ensure I respond in a timely fashion. :)

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.

4 participants