Skip to content

Move main menu data into python#1849

Closed
binary1230 wants to merge 1 commit intomasterfrom
move_menu_data_to_python
Closed

Move main menu data into python#1849
binary1230 wants to merge 1 commit intomasterfrom
move_menu_data_to_python

Conversation

@binary1230
Copy link
Copy Markdown
Contributor

@binary1230 binary1230 commented Jun 9, 2016

Previously, main menu data was in javascript and was slightly hard to modify from other plugins.

Now, other plugins can just append whatever they want to c.MENU, and there is logic in there to merge submenu sections intelligently.

We render this into JSON data via c.MENU_JSON (I am using a custom encoder so couldn't just |json it in the template)

This also opens up the future possibility of driving the main menu from some type of reflection process, like the sitemap currently does.

Previously, main menu data was in javascript and was slightly hard to modify from other plugins.

Now, other plugins can just append whatever they want to c.MENU, and there is logic in there to merge submenu sections intelligently.

This also opens up the future possibility of driving the main menu from some type of reflection process, like the sitemap currently does.
@binary1230
Copy link
Copy Markdown
Contributor Author

example of plugin adding a menu item to an existing menu:

image

@binary1230
Copy link
Copy Markdown
Contributor Author

@kitsuta
Copy link
Copy Markdown
Member

kitsuta commented Jun 9, 2016

Although I know the menu needs refactoring and it's clear that you put a bunch of work into this, I have some concerns about adding Yet Another Custom Class that people making plugins will have to memorize in order to use. I don't actually see that much gain in simplicity from "pushing to a dict" to "calling a custom function (in order to push to a dict)" - it's about the same except that the latter requires knowledge of custom syntax.

Was the problem that it was difficult to append to an existing submenu rather than adding a new one?

@binary1230
Copy link
Copy Markdown
Contributor Author

Couple of issues:

  1. Hard to append to menus from plugins because currently we don't have a way to automatically aggregate multiple blocks or templates from a bunch of plugins and combine into one result.
  2. Wanted to have the possibility in the future of not hiding menu items that you don't have access to, so that when you click on something you don't have access to it could either be greyed out or say "sorry you don't have access"
  3. Wanted to have the possibility in the future of not needing an explicit menu enumeration, but instead doing something similar to the sitemap where we could use reflection to populate this.

I chose to go the custom class route instead of just a simple dict for a few reasons:

  1. I wanted the ability to intelligently merge submenus and not have to worry about it carefully (i.e. appending a submenu to a menu item that only has one item should create a new submenu that has 2 items, one of them being the original item)
  2. Kinda disliked the menu data living in a javascript file
  3. I wanted to handle hierarchies of access control levels well, and have this be reflected in the exported menu data invisibly to the presentation layer

So while I do see that this is Yet Another Class, I feel like it's simple enough that it's not overburdening our plugin authors, it's pretty easy to look at what the main menu is doing in the main uber plugin and figure out what's needed to append menu items with minimal effort. I think it's a worthwhile tradeoff.

@binary1230
Copy link
Copy Markdown
Contributor Author

I'm not super-attached to this btw, though, I think it's an improvement on what we were doing before in terms of making it easier for plugin authors to add more stuff to the menus. This is sorely needed in particular with the band and analytics plugins. I'm fine with later changing it to something more simple if we feel like that's the right call.

I also was thinking, after talking to Chaney today, that it might be a good idea for styling purposes for this to not use javascript to render itself and move it all back to template logic - that's now a possibility because the data now lives on the backend instead of purely on the frontend.

@binary1230
Copy link
Copy Markdown
Contributor Author

hey FYI, I have a newer version of this (checked into a different branch) in which I was able to completely remove all menu processing from javascript and just do it with django templates, which is way cleaner and more friendly for re-styling later.

I need to clean that up a little bit more before it's ready, but, it looks like this on the template side:

{% block mainmenu %}

It also means the JSON stuff would be totally unnnecessary as I could just pass the raw python object, though I need to rewrite the filtering now to not go through JSON. easy stuff, just more work.

Would appreciate if we could come to definitive answer on this as some upcoming UI rewrites that @cybersenshi and I would be working on would heavily depend on some of this menu stuff going through.

@binary1230
Copy link
Copy Markdown
Contributor Author

This PR is now osboleted by #1857 which is functional, but needs a bit more cleanup before it's ready to go, let's redirect discussion there.

@binary1230 binary1230 closed this Jun 13, 2016
@kitsuta kitsuta deleted the move_menu_data_to_python branch May 15, 2017 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants