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

Convert Dropdown menu to component #2009

Merged
merged 5 commits into from Oct 19, 2017

Conversation

ZitaNemeckova
Copy link
Contributor

@miq-bot miq-bot changed the title Convert Dropdown menu to component [WIP] Convert Dropdown menu to component Aug 28, 2017
@miq-bot miq-bot added the wip label Aug 28, 2017
@ZitaNemeckova ZitaNemeckova force-pushed the dropdown_menu_component branch 5 times, most recently from 30cbe09 to 1fc3f9b Compare September 1, 2017 09:56
@ZitaNemeckova
Copy link
Contributor Author

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Convert Dropdown menu to component Convert Dropdown menu to component Sep 1, 2017
@miq-bot miq-bot removed the wip label Sep 1, 2017
' </button>' +
' <ul aria-labelledby="{{vm.dropdown_id}}" class="dropdown-menu dropdown-menu-right">' +
' <li ng-repeat="button in vm.buttons">' +
' <a id="{{button.id}}" title="{{button.title}}" data-method="{{button.dataMethod}}" data-remote="{{button.dataRemote}}" confirm="{{button.confirm}}" href="{{button.href}}" data-miq_spark_on="{{button.sparkOn}}">' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data-miq_sparkle_on ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, neither sparkOn nor sparkleOn is actually used .. looks like you just skipped them :)


this.$onInit = function() {
vm.dropdown_id = 'btn_' + vm.id;
vm.buttons = JSON.parse(vm.buttons);
Copy link
Contributor

@himdel himdel Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing $onChange probably .. or should the binding be @ since we're passing in a string?

unless @sb[:dashboards][@sb[:active_db]][:locked]
buttons.push(:id => "w_#{@widget.id}_close",
:title => _("Remove from Dashboard"),
:name => _(" Remove Widget"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra leading space in " Remove Widget"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... looks most of the name values have that - please remove it and make sure the component shows a reasonable space when both fonticon and name are used :).

(Most likely, adding a ng-if for fonticon and forcing fa-fw (fixed width) for the icon if it is there is what you want.)

' <ul aria-labelledby="{{vm.dropdown_id}}" class="dropdown-menu dropdown-menu-right">' +
' <li ng-repeat="button in vm.buttons">' +
' <a id="{{button.id}}" title="{{button.title}}" data-method="{{button.dataMethod}}" data-remote="{{button.dataRemote}}" confirm="{{button.confirm}}" href="{{button.href}}" data-miq_spark_on="{{button.sparkOn}}">' +
' <span class="{{button.fonticon}}"></span>' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ng-class

- if %w(chart).include?(presenter.widget.content_type)
%li
= presenter.button_zoom
%dropdown-menu{:id => 42, :buttons => presenter.widget_buttons}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

42? ;)

@ZitaNemeckova ZitaNemeckova force-pushed the dropdown_menu_component branch 2 times, most recently from b204549 to 49551e5 Compare September 29, 2017 13:17
vm.buttons = JSON.parse(vm.buttonsData);
};
},
template: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZitaNemeckova don't we want this in a static HTML?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or HAML 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%div.dropdown.pull-right.dropdown-kebab-pf
  %button.btn.btn-link.dropdown-toggle{'aria-haspopup' => "true",
                                       'data-toggle'   => "dropdown",
                                       'id'            => "{{vm.dropdown_id}}",
                                       'type'          => "button",
                                       'aria-expanded' => "false"}
    %span.fa.fa-ellipsis-v

  %ul.dropdown-menu.dropdown-menu-right{'aria-labelledby' => "{{vm.dropdown_id}}"}
    %li{'ng-repeat' => "button in vm.buttons"}
      %a{'id'                => "{{button.id}}",
         'title'             => "{{button.title}}",
         'data-method'       => "{{button.dataMethod}}",
         'data-remote'       => "{{button.dataRemote}}",
         'confirm'           => "{{button.confirm}}",
         'href'              => "{{button.href}}",
         'data-miq_spark_on' => "{{button.sparkleOn}}"}
        %span{'ng-class' => "button.fonticon"}
          {{button.name}}

@ZitaNemeckova
Copy link
Contributor Author

@miq-bot add_label wip

@miq-bot miq-bot changed the title Convert Dropdown menu to component [WIP] Convert Dropdown menu to component Oct 11, 2017
@miq-bot miq-bot added the wip label Oct 11, 2017
@ZitaNemeckova ZitaNemeckova force-pushed the dropdown_menu_component branch 3 times, most recently from fe0bee5 to f278fba Compare October 13, 2017 08:00
@ZitaNemeckova
Copy link
Contributor Author

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Convert Dropdown menu to component Convert Dropdown menu to component Oct 13, 2017
@miq-bot miq-bot removed the wip label Oct 13, 2017
'data-remote' => "{{button.dataRemote}}",
'confirm' => "{{button.confirm}}",
'href' => "{{button.href}}",
'data-miq_spark_on' => "{{button.sparkleOn}}"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

date-miq_sparkle_on ;)

Copy link
Contributor

@himdel himdel Oct 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also ... you're never setting sparkleOn to true, so please do :)

'title' => "{{button.title}}",
'data-method' => "{{button.dataMethod}}",
'data-remote' => "{{button.dataRemote}}",
'confirm' => "{{button.confirm}}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirm isn't working.. I'll try to find how it works normally..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update.. it's broken in master too :) but let's fix it.. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. indeed .. make this data-confirm and it works :).

@himdel
Copy link
Contributor

himdel commented Oct 17, 2017

Except for data-confirm and miq-sparkle-on, looks like this works just fine :) (I'm seeing the same buttons, causing the same errors 👍 .)

@ZitaNemeckova
Copy link
Contributor Author

@himdel Fixed. Thanks :)

'data-remote' => "{{button.dataRemote}}",
'data-confirm' => "{{button.confirm}}",
'href' => "{{button.href}}",
'data-miq_sparkle_on' => true}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this happen just for button close and button zoom?

@himdel
Copy link
Contributor

himdel commented Oct 18, 2017

(To address the problem of data-miq_sparkle_on="false" still sparkling.)

miq_ujs_bindings.js line 119:

$(document).on('focus', '[data-miq_observe]', function () {

I don't think you can catch it in the selector, but in the code itself, you could just do something like if (el.data('miq_observe') === "false") return; }.

EDIT: sorry, I confused sparkle and observe, but you found it already :)

@himdel
Copy link
Contributor

himdel commented Oct 19, 2017

Tested in the UI, merging when green :)

@miq-bot
Copy link
Member

miq-bot commented Oct 19, 2017

Checked commits ZitaNemeckova/manageiq-ui-classic@76e9d4d~...7a5787c with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@himdel himdel merged commit 1a9b222 into ManageIQ:master Oct 19, 2017
@himdel himdel added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 19, 2017
@ZitaNemeckova ZitaNemeckova deleted the dropdown_menu_component branch October 19, 2017 12:57
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

5 participants