Add wrapper for widget in Dashboard #3281

ZitaNemeckova · 2018-01-19T13:30:12Z

TODO:

Add wrapper
Resolve GET request failure
Remove all old code
Add spinner

Error widget:

Spinner widget (it spins):

@miq-bot add_label wip, dashboards, gaprindashvili/no

ZitaNemeckova · 2018-02-12T09:38:04Z

@himdel please have a look, thanks :)

himdel · 2018-02-13T13:17:16Z

app/views/dashboard/_widget.html.haml

+  %widget-wrapper{"widget-id"       => presenter.widget.id,
+                  "widget-type"     => presenter.widget.content_type,
+                  "widget-buttons"  => presenter.widget_buttons,
+                  "widget_blank"    => widget_blank,


widget-blank ;)

himdel · 2018-02-13T13:18:06Z

app/assets/javascripts/components/widget-wrapper.js

+    };
+  }],
+  template: [
+    '<div id="{{vm.divId}}">',


ng-attr-id for all those id={{.. please ;)

himdel · 2018-02-13T13:18:21Z

app/assets/javascripts/components/widget-wrapper.js

+    '        </h2>',
+    '      </div>',
+    '    </div>',
+    '      <widget-error ng-if="vm.error === true"></widget-error>',


whitespace..

ZitaNemeckova · 2018-02-19T12:32:44Z

@miq-bot remove_label wip

ZitaNemeckova · 2018-03-08T10:22:13Z

@karelhala please review, thanks :)

karelhala

Codewise looks good, however consider moving widget wrapper inside haml file and use angularTemplateUrl so we get HTML highlights.

Also @ZitaNemeckova it would be even better if this would have been moved to common pack if possible. So we get to use ES6 goodies and have more readable code.

karelhala · 2018-03-12T09:34:57Z

app/assets/javascripts/components/widget-footer.js

+  template: [
+    '<div class="card-pf-footer">',
+    __('Updated'),
+    "{{vm.widgetLastRun}}",


Please use single quotes.

karelhala · 2018-03-12T09:38:11Z

app/assets/javascripts/components/widget-menu.js

-    '        <td>',
-    '          <a title="' + __("Click to go this location") + '" href="{{shortcut.href}}">',
+    '    </div>',
+    '    <tr ng-if="!vm.shortcutsMissing()" ng-repeat="shortcut in vm.widgetModel.shortcuts">',


No need for the ng-if, if sortcuts has length of 0 ng-repeat will repeat zero times.

karelhala · 2018-03-12T09:41:02Z

app/assets/javascripts/components/widget-menu.js

+    '    </div>',
+    '    <tr ng-if="!vm.shortcutsMissing()" ng-repeat="shortcut in vm.widgetModel.shortcuts">',
+    '      <td>',
+    '        <a title="' + __("Click to go this location") + '" href="{{shortcut.href}}">',


You can use ng-attr-title to make it more readable. Also use ng-href so angular has time to work with it.

<a ng-attr-title="{{__("Click to go this location")}}" ng-href="{{shortcut.href}}">

karelhala · 2018-03-12T09:46:56Z

app/assets/javascripts/components/widget-wrapper.js

+    '    </div>',
+    '    <widget-error ng-if="vm.error === true"></widget-error>',
+    '    <widget-spinner ng-if="!vm.widgetModel && vm.widgetBlank == \'false\' && !vm.error"></widget-spinner>',
+    '    <div ng-if="vm.widgetBlank === \'true\' || vm.widgetModel" class="mc" id="{{vm.innerDivId}}" ng-class="{ hidden: vm.widgetModel.minimized }">',


You can join class and ng-class together and use ng-attr-id so angular has time to work correctly:

<div ng-if="vm.widgetBlank === \'true\' || vm.widgetModel" ng-attr-id="{{vm.innerDivId}}" ng-class="{ hidden: vm.widgetModel.minimized, mc: true }">

ng-attr-id

karelhala · 2018-03-12T09:48:08Z

app/assets/javascripts/components/widget-wrapper.js

+      }
+    };
+  }],
+  template: [


Wouldn't it be better to place this HTML inside static haml file and load is using template-url?

karelhala · 2018-03-12T09:57:40Z

app/assets/javascripts/components/widget-wrapper.js

+    vm.widgetUrl  = function() {
+      switch (vm.widgetType) {
+        case 'menu':
+          return '/dashboard/widget_menu_data/' + vm.widgetId;


Please use constants for these strings.

var dashboard = 'dashboard'; var widgetTypes = { menu: 'widget_menu_data'; report: 'widget_report_data'; chart: 'widget_chart_data'; rss: 'widget_rss_data'; } // ... var buildURL = function(type) { return ['dashboard', type, vm.widgetId].join('/'); }

And use it inside widgetUrl as

vm.widgetUrl = function() { if(widgetTypes.hasOwnProperty(vm.widgetType)) { return buildURL(widgetTypes[vm.widgetType]); } else { console.log('Something went wrong. There is no support for widget type of ', vm.widgetType); } }

It's preferred to have a whole url to make it easier to find it. That's why it's not pretty.

Maybe it still makes sense to use an object instead of a switch. But yes, please keep the whole url together, this will be a part of the component definition later.

ZitaNemeckova · 2018-03-14T11:30:00Z

@karelhala Agreed but I would prefer to move it in follow-up PR. And for moving HTML from components to haml it breaks specs because haml isn't compiled. Otherwise I fixed it as you suggested. Thanks :)

martinpovolny · 2018-04-08T19:25:11Z

@karelhala : Can we merge this one?

karelhala · 2018-04-09T12:23:46Z

@martinpovolny sure go ahead, I forgot to mark is as OK.

himdel · 2018-04-09T15:33:18Z

app/assets/javascripts/components/widget-menu.js

+    '    </div>',
+    '    <tr ng-repeat="shortcut in vm.widgetModel.shortcuts">',
+    '      <td>',
+    '        <a ng-attr-title="{{__("Click to go this location")}}" ng-href="{{shortcut.href}}">',


@ZitaNemeckova this was better as it used to be. You can't use __ inside of {{. (And no reason to use ng-attr-title for a static string.)

(Or you need to do $scope.__ = __; in this controller.)

himdel · 2018-04-09T15:38:51Z

app/assets/javascripts/components/widget-spinner.js

+  template: [
+    '<div class="blank-slate-pf" style="padding: 10px">',
+    '  <div class="blank-slate-pf-icon">',
+    '  <i class="fa fa-spin fa-spinner"></i>',


Missing 2 spaces ;)

just needed to include/mock all the dependencies and not use jquery selectors

himdel · 2018-04-10T12:22:52Z

app/assets/javascripts/components/widget-menu.js

    '      <td>',
-    '        <a title="' + __("Click to go this location") + '" href="{{shortcut.href}}">',
+    '        <a title="' +__("Click to go this location")+ '" ng-href="{{shortcut.href}}">',


Use ng-class, ng-attr-, ng-class, ng-href Move urls from switch to object (pretty)

Mock missing setupVerticalNavigation function from PatternFly

himdel · 2018-04-10T14:59:40Z

Tried clicking on the "Zoom in" button...

Error text:

Missing partial dashboard/_widget_footer, application/_widget_footer with {:locale=>[:en], :formats=>[:html], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby, :coffee, :haml, :rjs]}. Searched in: * "/home/himdel/manageiq-ui-classic/app/views" * "/home/himdel/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/bundler/gems/miq_v2v_ui_plugin-cd901ff708d5/app/views" [dashboard/widget_zoom]

So.. looks like you can't remove widget_footer it quite yet .. or better, you can convert the other use too ;)

app/views/dashboard/_zoomed_chart.html.haml
14:    = render :partial => 'widget_footer', :locals => {:widget => widget}

ZitaNemeckova · 2018-04-12T14:51:09Z

Fixed issue with footer (see here) and same id warnings.

Replace one partial call with widget-footer component, move lightbox-panel on same level as notification-app, move last_run and next_run to helper(no duplication of code) and fix same id warnings

miq-bot · 2018-04-12T14:57:36Z

Checked commits ZitaNemeckova/manageiq-ui-classic@7b67b85~...0c94918 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 3 offenses detected

**

💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

app/helpers/dashboard_helper.rb

❗ - Line 11, Col 95 - Layout/SpaceAroundOperators - Operator : should be surrounded by a single space.
❗ - Line 11, Col 96 - Layout/ExtraSpacing - Unnecessary spacing detected.

himdel · 2018-04-13T14:29:11Z

LGTM, I can press all the buttons now :)

The footer spacing around the " | " between the 2 dates changed a bit, but this is just less extra whitespace, no problem 👍

miq-bot changed the title ~~Add wrapper for widget in Dashboard~~ [WIP] Add wrapper for widget in Dashboard Jan 19, 2018

miq-bot added dashboards gaprindashvili/no wip labels Jan 19, 2018

ZitaNemeckova force-pushed the widget_cleanup branch from 3deda8f to a09044e Compare January 23, 2018 10:26

ZitaNemeckova force-pushed the widget_cleanup branch 2 times, most recently from 4287e38 to 6b5b2d0 Compare February 8, 2018 14:31

himdel reviewed Feb 13, 2018

View reviewed changes

ZitaNemeckova force-pushed the widget_cleanup branch 4 times, most recently from b1505a8 to 20b0b88 Compare February 19, 2018 12:30

miq-bot changed the title ~~[WIP] Add wrapper for widget in Dashboard~~ Add wrapper for widget in Dashboard Feb 19, 2018

miq-bot removed the wip label Feb 19, 2018

karelhala reviewed Mar 12, 2018

View reviewed changes

ZitaNemeckova force-pushed the widget_cleanup branch from b553931 to cce9f3d Compare March 14, 2018 10:17

karelhala approved these changes Apr 9, 2018

View reviewed changes

himdel reviewed Apr 9, 2018

View reviewed changes

ZitaNemeckova added 3 commits April 10, 2018 13:36

Add wrapper for widget in Dashboard

7b67b85

Remove no longer used views

30ad6a2

Add spinner when no data is loaded yet

13109eb

ZitaNemeckova and others added 8 commits April 10, 2018 13:36

Add error option and styling of spinner

079414c

Move specific parts of widget-wrapper to components

dcf173d

Minor fixes and specs

95d2657

Specs

c661b8a

Widget-* specs - make them work in jest :)

fe2ef47

just needed to include/mock all the dependencies and not use jquery selectors

minor fixes

1c0af0e

dashboard specs - common mocks and includes to a shared file

e4540c0

Fixes linter errors

42aac7f

ZitaNemeckova force-pushed the widget_cleanup branch from cce9f3d to 4495c35 Compare April 10, 2018 11:36

himdel reviewed Apr 10, 2018

View reviewed changes

ZitaNemeckova added 2 commits April 10, 2018 15:01

Fix according to review

e3dddf5

Use ng-class, ng-attr-, ng-class, ng-href Move urls from switch to object (pretty)

Fix failing jest spec

67cd442

Mock missing setupVerticalNavigation function from PatternFly

ZitaNemeckova force-pushed the widget_cleanup branch from 4495c35 to 67cd442 Compare April 10, 2018 13:02

himdel self-assigned this Apr 10, 2018

Fix widget_footer

0c94918

Replace one partial call with widget-footer component, move lightbox-panel on same level as notification-app, move last_run and next_run to helper(no duplication of code) and fix same id warnings

ZitaNemeckova force-pushed the widget_cleanup branch from 998735a to 0c94918 Compare April 12, 2018 14:53

himdel added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 13, 2018

himdel merged commit f9f90f9 into ManageIQ:master Apr 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wrapper for widget in Dashboard #3281

Add wrapper for widget in Dashboard #3281

ZitaNemeckova commented Jan 19, 2018 •

edited

Loading

ZitaNemeckova commented Feb 12, 2018

himdel Feb 13, 2018

himdel Feb 13, 2018

himdel Feb 13, 2018

ZitaNemeckova commented Feb 19, 2018

ZitaNemeckova commented Mar 8, 2018

karelhala left a comment •

edited

Loading

karelhala Mar 12, 2018

karelhala Mar 12, 2018

karelhala Mar 12, 2018

karelhala Mar 12, 2018

karelhala Mar 12, 2018

karelhala Mar 12, 2018

ZitaNemeckova Mar 12, 2018

himdel Mar 12, 2018

ZitaNemeckova commented Mar 14, 2018

martinpovolny commented Apr 8, 2018

karelhala commented Apr 9, 2018

himdel Apr 9, 2018 •

edited

Loading

himdel Apr 9, 2018

himdel Apr 10, 2018

himdel commented Apr 10, 2018 •

edited

Loading

ZitaNemeckova commented Apr 12, 2018

miq-bot commented Apr 12, 2018

himdel commented Apr 13, 2018 •

edited

Loading

Add wrapper for widget in Dashboard #3281

Add wrapper for widget in Dashboard #3281

Conversation

ZitaNemeckova commented Jan 19, 2018 • edited Loading

ZitaNemeckova commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZitaNemeckova commented Feb 19, 2018

ZitaNemeckova commented Mar 8, 2018

karelhala left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZitaNemeckova commented Mar 14, 2018

martinpovolny commented Apr 8, 2018

karelhala commented Apr 9, 2018

himdel Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Apr 10, 2018 • edited Loading

ZitaNemeckova commented Apr 12, 2018

miq-bot commented Apr 12, 2018

himdel commented Apr 13, 2018 • edited Loading

ZitaNemeckova commented Jan 19, 2018 •

edited

Loading

karelhala left a comment •

edited

Loading

himdel Apr 9, 2018 •

edited

Loading

himdel commented Apr 10, 2018 •

edited

Loading

himdel commented Apr 13, 2018 •

edited

Loading