-
Notifications
You must be signed in to change notification settings - Fork 487
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
Linking the conferences on home page to their splash page #97
Conversation
@@ -0,0 +1,5 @@ | |||
class ConferencesController < ApplicationController | |||
def show | |||
@conference = Conference.find_by_title(params[:format]) |
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.
Use 2 (not 1) spaces for indentation.
Tab detected.
@@ -0,0 +1,5 @@ | |||
class ConferencesController < ApplicationController | |||
def show | |||
@conference = Conference.find_by_title(params[:format]) |
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.
Use 2 (not 1) spaces for indentation.
Tab detected.
We already have a route for conferences with proposals, the schedule and registrations. Why do you introduce another one? |
I haven't setup the route but the view and the controller to kick start my Gopesh Tulsyan On Thu, May 8, 2014 at 3:23 PM, Henne Vogelsang notifications@github.comwrote:
|
What do you call your change to config/routes.rb then? :) |
oops sorry for the mistake :) So can i merge? Gopesh Tulsyan On Thu, May 8, 2014 at 5:14 PM, Henne Vogelsang notifications@github.comwrote:
|
Not like this no, please integrate "show" into the existing route. https://github.com/gopesht/osem/blob/event_splash_page/config/routes.rb#L52 |
@@ -0,0 +1,5 @@ | |||
class ConferenceController < ApplicationController | |||
def show | |||
@conference = Conference.find_by_title(params[:conference_id]) |
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.
Use 2 (not 1) spaces for indentation.
Tab detected.
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.
The param will be :id
here, not :conference_id
, as its not a nested resource. See the tables above and below http://guides.rubyonrails.org/routing.html#nested-resources .
@@ -48,6 +47,7 @@ | |||
end | |||
|
|||
resources :conference, :only => [] do | |||
get '/show' => 'conference#show' |
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 isn't necessary; use the :only option on resource instead:
resources :conference, :only => [:show]
See http://guides.rubyonrails.org/routing.html#restricting-the-routes-created
In addition to the inline comments, I expect you to add test coverage of your changes. |
@@ -0,0 +1,5 @@ | |||
class ConferenceController < ApplicationController | |||
def show | |||
@conference = Conference.find_by_title(params[:id]) |
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.
Use 2 (not 1) spaces for indentation.
Tab detected.
@@ -0,0 +1,38 @@ | |||
!!! |
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.
Why do you include layout code in this view?
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.
Because the view of the splash page will not contain headers and footers like other pages.It will have some custom header and footer which I will design.however, it will still contain all the code which is use to render the assets and basic html5 template.so i did a layout false in the controller, so that the application.html.haml is not rendered in this page.
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.
You should use your own splash layout then. Not have layout code in the view...
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.
okay so I need to put the layout in a seperate file.
This reverts commit 2a3410c.
First step towards event splash for visitors.Linking the events on home page to their respective splash pages.