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

vertical_navbar - a element to use exisitng param, href #3683

Merged
merged 1 commit into from Mar 28, 2018

Conversation

AllenBW
Copy link
Member

@AllenBW AllenBW commented Mar 26, 2018

In support of https://github.com/priley86/miq_v2v_ui_plugin/issues/161 but also its a bug.

cc @ZitaNemeckova for being awesome and helping me with this 🙇‍♀️ ❤️

@mzazrivec
Copy link
Contributor

@ZitaNemeckova Are these changes OK with you?

@himdel
Copy link
Contributor

himdel commented Mar 27, 2018

This looks wrong.

This should be %a{menu_item.link_params}, same as every other link in the menu.

(For ordinary menu items, the effect should be the same, but this also handles modal links, big iframe links, etc.)

@AllenBW
Copy link
Member Author

AllenBW commented Mar 27, 2018

So when yah do something along the lines of link_params and then the CustomLoader registers a menu item... if that menu item is not a section, onclick get redirected to something like this http://localhost:3000/ems_cloud/%7B:href=%3E%22/migration%22,%20:onclick=%3E%22return%20miqCheckForChanges();%22%7D 🙅‍♂️

(not something that happens with href, but I didn't check any modals either 😢 )

I see the definition though, link_params it it should be... this is how the menu is being created 🤔

      Menu::CustomLoader.register(
          Menu::Item.new('overview', N_('Migration'), 'miq_report', {:feature => 'miq_report', :any => true}, '/migration', :default, :compute)
      )

maybe instead of :default 🤔

nope that doesn't make a difference.

almost like we need to suppress default behavior... 🤔

@AllenBW
Copy link
Member Author

AllenBW commented Mar 27, 2018

OH it all makes sense now. Please disregard previous mental flailing.

Updated.

@miq-bot
Copy link
Member

miq-bot commented Mar 27, 2018

Checked commit AllenBW@1094c05 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@ZitaNemeckova
Copy link
Contributor

I tried it with https://github.com/priley86/miq_v2v_ui_plugin/pull/171 .
Before:

I, [2018-03-23T11:36:59.056145 #13863]  INFO -- :   Rendered plugins/manageiq-ui-classic/app/views/layouts/_vertical_navbar.html.haml (221.6ms)
I, [2018-03-23T11:36:59.056413 #13863]  INFO -- :   Rendered plugins/manageiq-ui-classic/app/views/layouts/_center_div_dashboard_no_listnav.html.haml (227.3ms)
I, [2018-03-23T11:36:59.056607 #13863]  INFO -- :   Rendered plugins/manageiq-ui-classic/app/views/layouts/_content.html.haml (232.8ms)
F, [2018-03-23T11:36:59.056910 #13863] FATAL -- : Error caught: [ActionView::Template::Error] undefined method `url' for #<Menu::Item:0x00007fe3c4b32b80>
manageiq-ui-classic/app/views/layouts/_vertical_navbar.html.haml:22:in `block (2 levels) in _plugins_manageiq_ui_classic_app_views_layouts__vertical_navbar_html

After:
screen shot 2018-03-28 at 10 32 04 am

So LGTM for manageiq-ui-classic 👍

@mzazrivec mzazrivec self-assigned this Mar 28, 2018
@mzazrivec mzazrivec added this to the Sprint 83 Ending Apr 9, 2018 milestone Mar 28, 2018
@mzazrivec mzazrivec merged commit 0275224 into ManageIQ:master Mar 28, 2018
@AllenBW AllenBW deleted the bug/vetnav-2ndtier-link branch March 28, 2018 12:20
@himdel
Copy link
Contributor

himdel commented May 20, 2018

#3963: Together with #3347 + #3726, fixes menu for v2v in gaprindashvili

Fixes:

        ActionView::Template::Error (undefined method `url' for #<Menu::Item:0x0000557a828f2698>):
                19:                 - menu_section.items.each do |menu_item|
                20:                   - if menu_item.visible? && menu_item.leaf?
                21:                     %li.list-group-item{:class => item_nav_class(menu_item)}
                22:                       %a{:href => menu_item.url, :onclick => 'return miqCheckForChanges()'}
                23:                         %span.list-group-item-value
                24:                           = _(menu_item.name)
                25:

        plugins/manageiq-ui-classic/app/views/layouts/_vertical_navbar.html.haml:22:in `block (2 levels) in _plugins_manageiq_ui_classic_app_views_layouts__vertical_navbar_html_haml___3292313873831275247_69929764576580'
        plugins/manageiq-ui-classic/app/views/layouts/_vertical_navbar.html.haml:19:in `each'
        plugins/manageiq-ui-classic/app/views/layouts/_vertical_navbar.html.haml:19:in `block in _plugins_manageiq_ui_classic_app_views_layouts__vertical_navbar_html_haml___3292313873831275247_69929764576580'
        plugins/manageiq-ui-classic/app/presenters/menu/manager.rb:19:in `block in menu'
        plugins/manageiq-ui-classic/app/presenters/menu/manager.rb:18:in `each'
        plugins/manageiq-ui-classic/app/presenters/menu/manager.rb:18:in `menu'
        plugins/manageiq-ui-classic/app/views/layouts/_vertical_navbar.html.haml:3:in `_plugins_manageiq_ui_classic_app_views_layouts__vertical_navbar_html_haml___3292313873831275247_69929764576580'

@himdel himdel mentioned this pull request May 20, 2018
27 tasks
@himdel himdel added the v2v label May 28, 2018
simaishi pushed a commit that referenced this pull request May 31, 2018
vertical_navbar - a element to use exisitng param, href
(cherry picked from commit 0275224)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit f90a99ae411c5cffcdb8a9c4cf6dd3dc77824e56
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Wed Mar 28 12:14:42 2018 +0200

    Merge pull request #3683 from AllenBW/bug/vetnav-2ndtier-link
    
    vertical_navbar - a element to use exisitng param, href
    (cherry picked from commit 0275224f51923f38650cefe5739356c8ed9bb742)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants