Hookable menus by deivid-rodriguez · Pull Request #1401 · decidim/decidim

deivid-rodriguez · 2017-05-24T20:43:11Z

🎩 What? Why?

Fixes #131.

The first commit is what I need for the assemblies feature so I don't have to monkeypatch any views in the plugin. Instead, I can add an initializer like this:

initializer "decidim_assemblies.menu" do
  Decidim.menu :menu do |menu|
    menu.item -> { I18n.t("menu.assemblies", scope: "decidim") },
              -> { decidim_assemblies.assemblies_path },
              position: 2.5,
              active: :inclusive
  end
end

I was continuing to complete all the specifications in #131 (deivid-rodriguez@c4ba1a8), but I decided to stop and grab some feedback first.

Also, I haven't looked closely but maybe this also helps with integrating the decidim-accountability plugin?

📌 Related Issues

Fixes Abstract menus so they're extensible by other engines #131.
Related to Assemblies (1st Iteration) #1346.
Related to Accountability global section decidim-archive/decidim-accountability#4.

📋 Subtasks

Basic menus.
Add conditional display parameter.
Add items with icons (~~separate icon_item DSL method?,~~ conditional behavior based on the presence of an icon_name parameter?).
Migrate system & admin menus.

📷 Screenshots (optional)

None.

👻 GIF

codecov · 2017-05-24T21:02:29Z

Codecov Report

Merging #1401 into master will increase coverage by 0.06%.
The diff coverage is 99.23%.

@@            Coverage Diff             @@
##           master    #1401      +/-   ##
==========================================
+ Coverage   95.42%   95.48%   +0.06%     
==========================================
  Files         428      436       +8     
  Lines        7365     7492     +127     
==========================================
+ Hits         7028     7154     +126     
- Misses        337      338       +1

josepjaume · 2017-05-25T07:19:37Z

Hi @deivid-rodriguez, this looks really nice!

Using lambdas is quite succint. I don't have a special preference about it, but in decidim we tend to use APIs like:

initializer "decidim_assemblies.menu" do
  Decidim.menu :menu do |menu|
    menu.item do |item|
      item.title I18n.t("menu.assemblies", scope: "decidim")
      item.path decidim_assemblies.assemblies_path
      item.position 2.5
      item.active :inclusive
    end
end

It feels a bit more expressive, but maybe it's me. Also, with this API, you can skip the lambdas because |item| can get evaluated in the context of the renderer - you just store the block that resolves into |item| and evaluate it later.

This technique can also be used in your approach though, with a bit of extra work.

Oh and, as always, super nice quality work you're doing. Thank you so much <3

deivid-rodriguez · 2017-05-25T12:07:08Z

@josepjaume Thanks for the feedback!

I like the API you initially proposed in decidim-archive/decidim-accountability#4 (comment) so I tried to follow it here. I do admit to it's a bit uglier with the lambdas and your new idea is cleaner in that regard.

I still like it because:

It resembles to the underlying link_to form, so it should be easy to grasp for Rails devs. I mean, two compulsory parameters, the first one being the label and the second one the url, and then a trailing hash of extra optional parameters.
It would easily allow to directly pass any extra unknown parameters to the underlying link_to without having to extend the API. For example for setting extra CSS classes for specific elements or things like that. Not sure how easy that would be in the alternative proposal. Maybe through some method_missing trickery, but if feels nicer to just have a hash of parameters in case we wanted to implement something like that.

So I'd like to keep this form if you don't have a strong opinion.

Also, admin menus come with a bit of extra icon candy (https://github.com/decidim/decidim/blob/master/decidim-admin/app/helpers/decidim/admin/application_helper.rb#L18-L24), we should probably shape the API to account for that. I thought of a separate API method icon_item, or an extra icon parameter to the item method, not sure if you have any other ideas or preferences here.

josepjaume · 2017-05-25T12:13:17Z

Okay then! I like it.

But maybe you could skil the lambdas if you built the menu on rendering time? Just store all the menu block and evaluate them afterwards. Does that sound like a bad idea? Would clean it a bit for sure, but maybe we'd be missing some opportunities...?

Also about the icon candy, not really sure about it. Would just specifying classes be enough?

deivid-rodriguez · 2017-05-25T12:23:55Z

But maybe you could skil the lambdas if you built the menu on rendering time? Just store all the menu block and evaluate them afterwards. Does that sound like a bad idea? Would clean it a bit for sure, but maybe we'd be missing some opportunities...?

Yeah, I'd also like to clean up the lambdas. I'm not sure if you would ever want to specifically evaluate the items at initialization time, can't think of something like that right now so your idea make sense to me... I'll experiment a bit more and see if I can put it into code.

Also about the icon candy, not really sure about it. Would just specifying classes be enough?

Mmmm not sure what you mean, like specifying a CSS class and showing the icon via CSS instead of via markup?

deivid-rodriguez · 2017-05-25T17:07:47Z

@josepjaume I pushed some changes here, now the API looks as you designed it! 😃

On a side note, I'm having problems to get some PR's built, see https://circleci.com/gh/decidim/decidim/1238 for example). It seems that the circle config is not correctly picked up (https://discuss.circleci.com/t/circleci-not-always-picking-up-config-yml/11921, maybe?). I wonder if this has to do with opening PR's from a forked external repo...

I see that I'm in the developers group of decidim, does that mean I have push access to https://github.com/decidim/decidim?

beagleknight · 2017-05-26T07:24:58Z

Hi @deivid-rodriguez !
I'm not sure if it's related to these options:

I think we may need to enable the second option but I am not really sure.

deivid-rodriguez · 2017-05-28T23:03:50Z

@beagleknight Maybe, I'm not sure either. It's fine though, it does not happen very often :)

josepjaume · 2017-05-29T08:46:45Z

This is so awesome! Can't wait to see it hooked to the entire system!

houndci-bot

Some files could not be reviewed due to errors: