Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

What's up with HighVoltage.content_path= ? #30

Closed
mhoran opened this Issue · 10 comments

3 participants

@mhoran

This accessor is lame. It modifies a class variable in the HighVoltage module. This makes it quite messy to have multiple HighVoltage controllers in an application, to for example serve up different static content at /about vs. /bananas.

In the previous release, one could simply define current_page and set layout in a subclass of HighVoltage::PagesController if a different content path or layout were desired. This has the benefit of allowing you to have multiple static paths, e.g. /about and /bananas, each with their own content.

Is there a reason that the accessor is now preferred over the previous behavior? It was also previously possible to set the layout simply by using the layout method in a subclass, but HighVoltage.layout= seems to be preferred now as well.

@mjankowski
Owner

I sort of see what you're saying, but can you provide more background on what you are trying to accomplish, what you want your routes/urls to be, what controller(s) you want to have, what layout(s) you have on various pages, etc?

@mhoran

Sure. Basically, I've hacked this to work with high_voltage 1.0.1. I've got two controllers. BroadcastLiveController and AboutController. Both descend from HighVoltage::PagesController. I discovered that I could override current_page as such:

def current_page
  "about/#{clean_path}"
end

Not the most elegant solution, but it works. I did the same for the BroadcastLive controller, with the path prefix being broadcast-live. I'm using the standard layout method to set the about layout and broadcast layout. Of course I could use a method to assign the layout as well.

My routes are as follows:

match "about/*id" => "about#show", :via => :get, :as => :about
match "broadcast-live/*id" => "broadcast_live#show", :via => :get, :as => :broadcast_live

This allows me to use about_path and broadcast_live_path throughout the site, linking to the static content at /about/ and /broadcast-live/.

Note that there's an issue with the exception handling in HighVoltage::PagesController in version 1.0.1 where ActionView::MissingTemplate is only rescued if the template is in the pages directory. I've monkey patchd this myself.

I think the most elegant solution here would be to have a content_path method that is defined in your overridden controller, used in both the current_page and rescue_from, allowing the above extensions as well as other fancy behavior as defined by ActionController.

@jferris
Owner

Most people just have one controller and no dynamic behavior for the layout or content path, so those accessors are kind of handy. It's easier to just configure something in an initializer rather than subclassing and overriding methods.

You can still override the layout and current_page in the layout, can't you?

@mhoran

Yes, it's still possible to override the layout and current_page. However, this is a bit unnatural, since really you want to just define the base content path per overridden controller. Overrides in this case still have to append the clean_path to the base content path.

Also, because of the static path within the rescue_from of HighVoltage::PagesController (and in master, HighVoltage.content_path), I have to implement this entire block in overridden controllers.

If instead there were a content_path method, which as implemented by the gem just referenced HighVoltage.content_path, and could be overridden in subclasses, then we could avoid this problem by using content_path in the rescue_from. This makes it possible to use HighVoltage.content_path, and also subclass and redefine content_path on a per controller basis.

@mjankowski
Owner

Is it possible for you to route both of these to the same controller (regexp in routes?) and use a controller method to select layout and some view dir structure like app/views/pages/about/... and another with the broadcast-live views?

@jferris
Owner

If you can refactor this so that you can use the accessors or override it the way you want, that would be a sweet pull request.

@mhoran

@jferris, will do.

@mhoran

I've got this working, however I ran into a funky issue with the tests. Because HighVoltage.content_path= modifies global state, the tests are dependent on the order in which they run. I noticed this when all the tests were passing on my work computer, but failing at home. I traced this down to the routes_spec running before the pages_controller_spec, which breaks some specs in pages_controller_spec because routes_spec sets content_path.

This is what I was originally commenting on being lame. What would your preferred course of action be here? I can of course set up an after block, which resets content_path whenever it's changed, but this doesn't seem great either.

@jferris
Owner

Yeah, global state is harder to test. If the tests that are failing can use local state instead, I'd do that; if not, the after block will work fine.

@jferris
Owner

I'm closing this issue now, since the relevant pull request was merged.

@jferris jferris closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.