Replaced menu partials with Refinery::Pages::MenuPresenter. #2069
Conversation
|
@ugisozols for bonus points can you please rewrite the current specs that basically just check the menu's output to unit tests? 👍 |
|
@parndt sounds like a nice example of therapeutic refactoring for you! |
|
@keithpitty the partials are dead; long live the presenter! So happy. |
| :dom_id => 'menu', | ||
| :nav => :nav, | ||
| :ul => :ul, | ||
| :li => :li, |
There was a problem hiding this comment.
For one, why not just make it a private attr_reader? A class having to read its attributes out of a hash seems weird to me.
Second, allowing customizability of the specific tag that gets used seems of little utility. I'd replace line 33 with a method that rendered the list item to allow a subclass of MenuPresenter to customize list items if it were so desired.
|
I wanted to do this for a long time. Good work @parndt! 🍊 |
|
@phiggins updated a couple things; great idea about the |
Real subtle. |
|
Totally |
|
Good feedback, @phiggins ! Now we just need to move the specs that have to do with the menu only from here https://github.com/refinery/refinerycms/blob/master/pages/spec/requests/refinery/pages_spec.rb into unit tests for https://github.com/refinery/refinerycms/blob/menu_presenters_instead/pages/lib/refinery/pages/menu_presenter.rb |
|
So we have an issue whereby we've hit a limitation when having a many-deep nested set it hits SQL quite a lot. I've discussed this with @simi at great length and we haven't come up with a solution yet. It's not a new issue but it's something that we should really deal with. One idea I had after waking up is that we could delegate URL building to some other module that Page (but actually MenuItem) could delegate to. This would mean menu_items would need to have smarter attributes for to_param to be in there. And the menu would have to be instructed to build a nested URL or something. |
| page.has_children? && page.descendants.any?(&method(:selected_page?)) | ||
| end | ||
|
|
||
| def selected_page_or_descendant_page_selected?(page) |
There was a problem hiding this comment.
This is called with menu_item as a parameter. Should we continue to use that name throughout these private methods for clarity about what class the parameters are?
This has many advantages not least of which is that we can test it without requesting pages. * Using config_accessor and attr_accessor instead of passing options around. * Added leaf? to MenuItem and made use of has_children? in the MenuPresenter * All methods besides initialize and to_html are private to this class. * Moved Refinery::Core#menu_css to the Refinery::Pages::MenuPresenter. * Uses MenuItem#depth instead of scanning ancestors. * Renamed levels to max_depth and added within_max_depth? * Refactored Menu and MenuItem as proper objects, not pretending to be hashes or arrays (ht: @awagener) * Move Menu#initialize logic to Menu#append so that more items can be appended after initialize. * Moved all page presenters and their specs under app/presenters and spec/presenters where they belong.
Replaced menu partials with Refinery::Pages::MenuPresenter.
|
are there any benchmarks for this change? |
|
Benchmarks generally lie ;-) so I this ran myself in development, test and production modes with lots of differently sized menus and the performance is very good, save for the problems identified in issue #2087 which are unrelated to this refactor and were already an issue. On my system, which is just a 13" Macbook Pro, the performance was around 8ms in development and 3ms in production mode to render the entire menu of as many pages as you like. This performance declines as more pages are nested below other pages but this was an existing problem which, I think, is now much easier to solve and if not in 2.1.0 then it can be relatively transparently solved in 2.1.1, 2.1.2 etc without a change required to the public API of the presenter ( Customising the menu and upgrading between different versions is now much easier because you only have to override single methods (i.e. The point that menu partial was one of the most popular templates to override made it such a good candidate for refactor. I've dealt with so, so many applications which have had this overridden and then when it came time to upgrade this was the place that had the most errors. It was a complete pain to try to figure out the differences between the versions whereas now it is much cleaner and easier. If you really wanted, you could ignore the presenter completely and still keep using the old partial approach (just override header, menu and menu_branch from 2-0-stable and they will continue to work) but this is not shipping by default with Refinery anymore. Do you think I should put the partials back in with a deprecation warning for any applications that are upgrading and have the header overridden? I figured this wouldn't work because the user would most likely also have both menu partials overridden anyway so no notice would be seen anyway. This will be a major point highlighted in the upgrade notes and any blog post we write about the release of 2.1.0. No doubt we'll still have lots of questions on the user group but we always do! :-) I'm much, much happier with the menu presenter than I ever was with the menu partials, which I've been unhappy about since well before version 1.0. I hope that I've explained my point of view without rambling too much. Let me know if I can improve on anything that I've said and I'm also interested to hear your further thoughts on this. Thanks! 👍 |
|
@parndt here it's http://refinerycms.com/guides/menu-presenter There is even StackOverflow thread http://stackoverflow.com/questions/14759712/rails-bootstrap-navbar-and-refinerycms with few different ways how to override simple menu. |
|
@pdostal How custom does it need to be? I've managed to make a perfectly functional Bootstrap menu by only changing MenuPresenter's config in an initializer. If you need custom contents in your menu there's https://github.com/isaacfreeman/refinerycms-menus/tree/rails-4-1(the branch that works with Rails 4 and Refinery master). To be honest, Gitter or IRC are probably better places to ask questions. |
|
@gwagener It wasn't question, I just said that it isn't easy for me. But thanks for your advice. |
This has many advantages not least of which is that we can test it without requesting pages.
@ugisozols @robyurkowski Merges on top of #2068 if you guys approve.