-
Notifications
You must be signed in to change notification settings - Fork 357
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
Convert Dropdown menu to component #2009
Conversation
30cbe09
to
1fc3f9b
Compare
@miq-bot remove_label wip |
' </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}}">' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-miq_sparkle_on ;)
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
app/presenters/widget_presenter.rb
Outdated
unless @sb[:dashboards][@sb[:active_db]][:locked] | ||
buttons.push(:id => "w_#{@widget.id}_close", | ||
:title => _("Remove from Dashboard"), | ||
:name => _(" Remove Widget"), |
There was a problem hiding this comment.
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"
?
There was a problem hiding this comment.
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>' + |
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
42? ;)
b204549
to
49551e5
Compare
vm.buttons = JSON.parse(vm.buttonsData); | ||
}; | ||
}, | ||
template: [ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or HAML 😉
There was a problem hiding this comment.
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}}
@miq-bot add_label wip |
fe0bee5
to
f278fba
Compare
@miq-bot remove_label wip |
'data-remote' => "{{button.dataRemote}}", | ||
'confirm' => "{{button.confirm}}", | ||
'href' => "{{button.href}}", | ||
'data-miq_spark_on' => "{{button.sparkleOn}}"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date-miq_sparkle_on ;)
There was a problem hiding this comment.
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}}", |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.. :)
There was a problem hiding this comment.
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 :).
Except for data-confirm and miq-sparkle-on, looks like this works just fine :) (I'm seeing the same buttons, causing the same errors 👍 .) |
f278fba
to
579d7ec
Compare
@himdel Fixed. Thanks :) |
'data-remote' => "{{button.dataRemote}}", | ||
'data-confirm' => "{{button.confirm}}", | ||
'href' => "{{button.href}}", | ||
'data-miq_sparkle_on' => true} |
There was a problem hiding this comment.
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?
(To address the problem of
I don't think you can catch it in the selector, but in the code itself, you could just do something like EDIT: sorry, I confused sparkle and observe, but you found it already :) |
Spinner is called for every other possible value including empty string, no value, ...
dfafd0b
to
7a5787c
Compare
Tested in the UI, merging when green :) |
Checked commits ZitaNemeckova/manageiq-ui-classic@76e9d4d~...7a5787c with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
@miq-bot add_label wip
https://www.pivotaltracker.com/story/show/147760695