-
Notifications
You must be signed in to change notification settings - Fork 43
Marisa Morris - Carets #27
base: master
Are you sure you want to change the base?
Conversation
|
Hey Marisa, I've reviewed your Media Ranker OAuth project and I have a bit of feedback. The good:
So you have the basics down. Things to work on:
I'm guessing you ran out of some time here. So not bad, but there was more to work though. In what little we have left of Rails and the classroom content, more practice with testing would be valuable. |
CheezItMan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few notes
| # We should always be able to tell what category | ||
| # of work we're dealing with | ||
| before_action :category_from_work, except: [:root, :index, :new, :create] | ||
| skip_before_action :find_user, only: [:root] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By avoiding this filter it shows a log in button on the homepage, even after I'm logged in. You can add a different bit of ocood to the root path to find the user. Or create 2 filters, one to find the user and another to redirect if it's not there.
| <!-- --------------------------------- --> | ||
| <div class="float-right"> | ||
| <!-- T0DO --> | ||
| <%= @login_user %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something to remove.
No description provided.