Skip to content
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

fixes #3790 - Added support for isolated engine to foreman menu. #1082

Closed
wants to merge 1 commit into from

Conversation

abenari
Copy link
Member

@abenari abenari commented Dec 8, 2013

Isolated engines needs to run the route helpers in the plugin context. Since the menu is common to the plugins and the main app, menu items urls needs to be computed at the correct context.
This patch add option to the 'menu' method.
An isolated engine needs to declare

 :engine => MyPlugin::Engine

for each menu item.
I was experimenting with declaring an engine is isolated instead of repeating the declaration for each menu item but favored the proposed solution because it lets an isolated plugin writer to interact with foremans controllers too.

This pull request replaces PR #1075

@domcleal
Copy link
Contributor

domcleal commented Dec 9, 2013

@ehelms @isratrade could you review please? We've got three PRs now for the same bug :)

@ehelms
Copy link
Member

ehelms commented Dec 9, 2013

Testing my usual scenarios with this.

@ehelms
Copy link
Member

ehelms commented Dec 9, 2013

From testing confirms that this should handle the issues with declaring a menu from an isolated engine, as well as the issue of rendering the menu when inheriting the Foreman layout. There were a few edge case items related to the structure of the Foreman layout, and more modifications on the Katello side I had to do to get a page to render. That being said, this looks good to me for now and we can address any other issues that may arise as I build out the rest of the Katello pages.

@ehelms
Copy link
Member

ehelms commented Dec 9, 2013

ACK for me. I'd also like the divider functionality, but prefer'd I will open a separate issue and include the addition there.

@domcleal
Copy link
Contributor

Looks good combined with #1055, though one more small issue: the top left Foreman "brand" logo uses root_path, but this should be main_app.root_path.

@ehelms
Copy link
Member

ehelms commented Dec 11, 2013

@domcleal good catch, forgot I was also seeing that when using Katello

@domcleal
Copy link
Contributor

Thanks @abenari, merged as 49a9808 for Foreman 1.4.0.

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.

3 participants