Containers Provider Dashboard conversion to use Angular/PF components #2499

h-kataria · 2017-10-24T13:27:20Z

before:

after:

https://bugzilla.redhat.com/show_bug.cgi?id=1609345

miq-bot · 2017-10-25T13:28:57Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-12-04T14:34:37Z

This pull request is not mergeable. Please rebase and repush.

epwinchell · 2018-01-22T22:11:34Z

@martinpovolny @himdel I recall we left off with this, wondering if the charts could be dynamically resized like the main dashboard. We'd like to do the same thing for all of the new angular dashboards. Is that something you could look at?

I see for Cap & U it is handled here: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/assets/javascripts/miq_c3.js

h-kataria · 2018-01-24T21:03:59Z

@epwinchell please review commit with styling changes, i hope i didn't miss any from the patch you sent me.

epwinchell · 2018-01-25T19:00:33Z

@h-kataria Tested. Looks fine.

miq-bot · 2018-01-30T17:12:19Z

This pull request is not mergeable. Please rebase and repush.

h-kataria · 2018-02-02T22:17:33Z

@himdel can you please review, let me know if this is still a valid/acceptable way to move forward with dashboard componentization work.

h-kataria · 2018-02-27T16:11:02Z

@himdel please review, would be good before more of similar ones get merged and are being worked on, another one in this area is #3427

h-kataria · 2018-07-26T17:29:50Z

@himdel please take a look, i have fixed the aggregate status card to show total counts/link of Providers on Containers Dashboard. This PR only affects All Containers Dashboard and individual Container Provider dashboard, all charts before/after on the above mentioned 2 types of dashboards look same to me. The only difference is layouts of charts which is intentional, goal is to keep all charts to same size so when we introduce drag/drop feature in future it wont cause any issues with regards to layouts.

h-kataria · 2018-07-26T17:38:01Z

Containers Dashboard:
before:

after:

Container Provider Dashboard:
before:

after:

Fixed existing spec test

himdel · 2018-07-27T10:55:40Z

The only difference is layouts of charts which is intentional, goal is to keep all charts to same size so when we introduce drag/drop feature in future it wont cause any issues with regards to layouts.

Aah, makes sense, thanks :).

Taking another look..

himdel · 2018-07-27T11:13:46Z

Issue I can still see:

container detail - network utilization trend missing a chart - sometimes
container detail - provider missing name
~~container detail - node utilization has no data (looking why, your screenshot looks fine)~~
container detail - numbers in alerts have wrong font (and the number should be there twice?)
dashboard - network utilization trend missing a chart - sometimes
dashboard - numbers with providers and alert have wrong font
~~dashboard - node utilization has no data (looking why, your screenshot looks fine)~~

EDIT: missing node utilization was a local problem, old metrics
EDIT2: network utilization trend ... sometimes it displays the chart, sometimes it doesn't, reloading can change it, so likely a timing problem

himdel · 2018-07-27T11:18:09Z

Oh, but the network utilization trend issue is probably easy, I can see an error in the console:

angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:14801 TypeError: Cannot read property 'layout' of undefined
    at trendsChartController.vm.updateAll (trends-chart.component.self-15211e0b82fd6c4300f468726085cf159aafb86322fe040f0353edfaad30e661.js:27)
    at trendsChartController.vm.$onChanges (trends-chart.component.self-15211e0b82fd6c4300f468726085cf159aafb86322fe040f0353edfaad30e661.js:59)
    at angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:9960
    at forEach (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:417)
    at nodeLinkFn (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:9956)
    at angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:10348
    at processQueue (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:17170)
    at angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:17218
    at Scope.$digest (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:18353)
    at Scope.$apply (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:18650)
(anonymous) @ angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:14801
angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:14801 TypeError: Cannot read property 'layout' of undefined
    at trendsChartController.vm.getChartHeight (trends-chart.component.self-15211e0b82fd6c4300f468726085cf159aafb86322fe040f0353edfaad30e661.js:52)
    at fn (eval at compile (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:15652), <anonymous>:4:184)
    at initializeBinding (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:10733)
    at forEach (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:417)
    at initializeDirectiveBindings (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:10640)
    at nodeLinkFn (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:9943)
    at angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:10348
    at processQueue (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:17170)
    at angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:17218
    at Scope.$digest (angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:18353)
(anonymous) @ angular.self-4d4ad11d5ffcf4b45937b602ad6794ec0402cf414a488a6e28edf2a00bb8370c.js:14801

himdel · 2018-07-27T12:19:22Z

app/views/ems_container/_heatmap.html.haml

+      .spinner.spinner-lg.loading{"ng-if" => "!vm.loadingDone"}
+      .row
+        .col-xs-12.col-sm-12.col-md-12.col-lg-6.example-heatmap-container{"ng-repeat" => "(key, data) in vm.data"}
+          %pf-heatmap{'ng-attr-id' => "{{data[0].id}}", "chart-title" => "key", "data" => "data", "chart-data-available" => "vm.dataAvailable", "show-legend" => "vm.showLegends", "legend-labels" => "vm.legendLabels", "max-block-size" => "20", "block-padding" => "5", "heatmap-color-pattern" => "vm.heatmapColorPattern", "thresholds" => "vm.thresholds", "click-action" => "vm.clickAction", "loading-done" => true}


"chart-data-available" => "vm.dataAvailable"

I think that's the problem - dataAvailable is true before the loading is done .. looks like you may want to use vm.dataAvailable && vm.loadingDone here?

himdel · 2018-07-27T12:22:34Z

As for the font problem, that looks like a bug in the existing app/views/static/pf_charts/aggregate-status-card.html.haml: The element with a fonticon class should never have text descendants, only siblings.

Soo..

diff --git a/app/views/static/pf_charts/aggregate-status-card.html.haml b/app/views/static/pf_charts/aggregate-status-card.html.haml
index ab86948cf..422cf8d21 100644
--- a/app/views/static/pf_charts/aggregate-status-card.html.haml
+++ b/app/views/static/pf_charts/aggregate-status-card.html.haml
@@ -23,12 +23,12 @@
         %a{:href => "{{notification.href}}", "ng-if" => "notification.href"}
           %image{"ng-if" => "notification.iconImage", "ng-src" => "{{notification.iconImage}}", :alt => "", :class => "card-pf-icon-image"}
           %span{:class => "{{notification.iconClass}}"}
-            {{ notification.count }}
+          {{ notification.count }}
 
         %span{"ng-if" => "!notification.href"}
           %image{"ng-if" => "notification.iconImage", "ng-src" => "{{notification.iconImage}}", :alt => "", :class => "card-pf-icon-image"}
           %span{:class => "{{notification.iconClass}}"}
-            {{ notification.count }}
+          {{ notification.count }}
 
 %div{"ng-if" => "$ctrl.isMiniLayout", :class => "card-pf card-pf-aggregate-status card-pf-aggregate-status-mini", "ng-class" => "{'card-pf-accented': $ctrl.shouldShowTopBorder}"}
   %h2{:class => "card-pf-title"}

Should be closer

himdel · 2018-07-27T12:24:50Z

Indeed, looks like with those 2 changes (aggregate and loadingDone):

alerts looks good
there is no console error
network utilization trend always shows up :)

h-kataria · 2018-07-27T13:05:01Z

@himdel, i will implement your suggestions and push those up in a few mins.

h-kataria · 2018-07-27T13:11:52Z

@himdel for container detail - provider missing name -It has been a long time since this PR was created but as far as i remember removing the name was intentional as the name is already available in the breadcrumbs

h-kataria · 2018-07-27T13:18:17Z

@himdel for container detail - numbers in alerts have wrong font (and the number should be there twice?) - it doesn't make sense to show same data/numbers twice on the card.

Fixed heatmap loading so it loads charts everytime properly.

miq-bot · 2018-07-27T13:27:50Z

Checked commits h-kataria/manageiq-ui-classic@b630035~...c369f5c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
15 files checked, 1 offense 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.

h-kataria · 2018-07-27T14:23:36Z

@himdel pushed a commit with suggested changes, all charts seem to be loading fine for me with the data i have. jest spec test failure seems to be unrelated to my changes.

himdel · 2018-07-27T14:26:25Z

Agreed, seeing the same thing :)

I think we're ready, sorry it took so long :).

Confirming the jest failure is unrelated - #4371.

h-kataria · 2018-07-27T15:12:44Z

@himdel thx for the merge.

felipedf · 2018-07-30T13:47:23Z

app/assets/javascripts/controllers/ems_common/charts-mixin.js

@@ -1,145 +0,0 @@
-angular.module('miq.util').factory('chartsMixin', ['$document', function($document) {


Any reason for keeping this empty file?

felipedf · 2018-07-30T13:47:30Z

app/assets/javascripts/controllers/mixins/charts-mixin.js

@@ -1,4 +1,4 @@
-angular.module('miq.util').factory('containerChartsMixin', ['pfUtils', function(pfUtils) {


So we are using this file over ems_common/charts-mixin, that correct? because I noticed that this file is missing few configs that were present on the older one.

chartsMixin should be preferred. If this PR removed chartsMixin and replaced it with an older version, that's a mistake (bad rebase I'd guess?).

(But looks like we had 2 versions already for some reason - mixins/charts-mixin and ems_common/charts-mixin. All of those should definitely be unified.)

You are right @himdel, I've heard about plans to unify the container with the other providers, so I won't touch it 😅, will just rollback the chartsMixin

Thanks @felipedf .. feel free to touch it enough so that you don't see any more breakages ;)

I'd love to unify these more, but.. obviously not breaking stuff is more important :)

Ok @himdel will try to help whenever I can (:

Actually, I guess it was exactly what @h-kataria did here, container_dashboard/util/charts-mixin was merged with the older chartsMixin but some configs might me lost in the between, will just update the new chartsMixin.

can confirm that @h-kataria ?

@felipedf i tried to keep a common charts_mixin for all charts in https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/assets/javascripts/controllers/mixins/charts-mixin.js

@felipedf apologies if during all the back and forth rebasing i missed/lost some piece of code, let me know i will be happy to help with recovering those.

@h-kataria it is fine 😄
I got it all working just fine now.. I was confused after pulling from master and ending up with 3 charts-mixin in my local.. i was like LOL 😅

the file is empty, and never referenced (emptied and last reference removed in ManageIQ#2499)

h-kataria added compute/containers enhancement wip labels Oct 24, 2017

miq-bot added the unmergeable label Oct 25, 2017

h-kataria force-pushed the containers_dashboard branch from 74a0235 to ffac729 Compare November 9, 2017 19:46

miq-bot removed the unmergeable label Nov 9, 2017

h-kataria force-pushed the containers_dashboard branch 2 times, most recently from a52d2c5 to 6c99339 Compare November 9, 2017 20:26

zeari mentioned this pull request Nov 19, 2017

Change metric accuracy in container projects dashboard #2762

Merged

miq-bot added the unmergeable label Dec 4, 2017

h-kataria force-pushed the containers_dashboard branch from 4db9581 to 812d450 Compare January 22, 2018 20:34

miq-bot removed the unmergeable label Jan 22, 2018

h-kataria force-pushed the containers_dashboard branch 2 times, most recently from 07aa2d2 to 6da4a1d Compare January 24, 2018 21:02

h-kataria changed the title ~~[WIP] - Containers Provider Dashboard conversion to use Angular/PF components~~ Containers Provider Dashboard conversion to use Angular/PF components Jan 24, 2018

h-kataria added gaprindashvili/no and removed wip labels Jan 24, 2018

miq-bot added the unmergeable label Jan 30, 2018

h-kataria force-pushed the containers_dashboard branch from 6da4a1d to 0f09aac Compare February 27, 2018 16:07

miq-bot removed the unmergeable label Feb 27, 2018

h-kataria force-pushed the containers_dashboard branch 2 times, most recently from 0429c8f to 3c6a37a Compare February 27, 2018 18:51

h-kataria force-pushed the containers_dashboard branch from 60905ee to e572339 Compare July 26, 2018 22:03

Show count and link on Containers Dashboard view in aggregate card.

e50a3be

Fixed existing spec test

h-kataria force-pushed the containers_dashboard branch from e572339 to e50a3be Compare July 26, 2018 22:18

himdel reviewed Jul 27, 2018

View reviewed changes

Fixed font issue on aggregate status card

c369f5c

Fixed heatmap loading so it loads charts everytime properly.

himdel merged commit 5763edb into ManageIQ:master Jul 27, 2018

himdel added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 27, 2018

felipedf reviewed Jul 30, 2018

View reviewed changes

himdel mentioned this pull request Jul 31, 2018

Adding some required config to charts-mixin #4381

Merged

epwinchell mentioned this pull request Aug 6, 2018

restore dashboard background color #4417

Merged

h-kataria deleted the containers_dashboard branch October 24, 2018 22:08

himdel mentioned this pull request Feb 8, 2019

Container dashboard utilization metrics - fix hourly metrics #5232

Merged

himdel mentioned this pull request Jun 6, 2019

Remove empty unused container_providers_dashboard.scss #5680

Merged

Hyperkid123 pushed a commit to Hyperkid123/manageiq-ui-classic that referenced this pull request Jun 11, 2019

Remove empty unused container_providers_dashboard.scss

85fa995

the file is empty, and never referenced (emptied and last reference removed in ManageIQ#2499)

himdel mentioned this pull request Jul 7, 2020

Utilization/Trends are shown on the container provider level but not on the projects level #7186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Containers Provider Dashboard conversion to use Angular/PF components #2499

Containers Provider Dashboard conversion to use Angular/PF components #2499

h-kataria commented Oct 24, 2017 •

edited

Loading

miq-bot commented Oct 25, 2017

miq-bot commented Dec 4, 2017

epwinchell commented Jan 22, 2018 •

edited

Loading

h-kataria commented Jan 24, 2018

epwinchell commented Jan 25, 2018

miq-bot commented Jan 30, 2018

h-kataria commented Feb 2, 2018

h-kataria commented Feb 27, 2018

h-kataria commented Jul 26, 2018

h-kataria commented Jul 26, 2018

himdel commented Jul 27, 2018

himdel commented Jul 27, 2018 •

edited

Loading

himdel commented Jul 27, 2018

himdel Jul 27, 2018

himdel commented Jul 27, 2018

himdel commented Jul 27, 2018

h-kataria commented Jul 27, 2018

h-kataria commented Jul 27, 2018

h-kataria commented Jul 27, 2018

miq-bot commented Jul 27, 2018

h-kataria commented Jul 27, 2018

himdel commented Jul 27, 2018

h-kataria commented Jul 27, 2018

felipedf Jul 30, 2018

felipedf Jul 30, 2018 •

edited

Loading

himdel Jul 30, 2018 •

edited

Loading

felipedf Jul 30, 2018

himdel Jul 30, 2018

felipedf Jul 30, 2018

felipedf Jul 30, 2018

h-kataria Jul 30, 2018

h-kataria Jul 30, 2018

felipedf Jul 30, 2018 •

edited

Loading

		@@ -1,145 +0,0 @@
		angular.module('miq.util').factory('chartsMixin', ['$document', function($document) {

		@@ -1,4 +1,4 @@
		angular.module('miq.util').factory('containerChartsMixin', ['pfUtils', function(pfUtils) {

Containers Provider Dashboard conversion to use Angular/PF components #2499

Containers Provider Dashboard conversion to use Angular/PF components #2499

Conversation

h-kataria commented Oct 24, 2017 • edited Loading

miq-bot commented Oct 25, 2017

miq-bot commented Dec 4, 2017

epwinchell commented Jan 22, 2018 • edited Loading

h-kataria commented Jan 24, 2018

epwinchell commented Jan 25, 2018

miq-bot commented Jan 30, 2018

h-kataria commented Feb 2, 2018

h-kataria commented Feb 27, 2018

h-kataria commented Jul 26, 2018

h-kataria commented Jul 26, 2018

himdel commented Jul 27, 2018

himdel commented Jul 27, 2018 • edited Loading

himdel commented Jul 27, 2018

Choose a reason for hiding this comment

himdel commented Jul 27, 2018

himdel commented Jul 27, 2018

h-kataria commented Jul 27, 2018

h-kataria commented Jul 27, 2018

h-kataria commented Jul 27, 2018

miq-bot commented Jul 27, 2018

h-kataria commented Jul 27, 2018

himdel commented Jul 27, 2018

h-kataria commented Jul 27, 2018

Choose a reason for hiding this comment

felipedf Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

himdel Jul 30, 2018 • 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

felipedf Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

h-kataria commented Oct 24, 2017 •

edited

Loading

epwinchell commented Jan 22, 2018 •

edited

Loading

himdel commented Jul 27, 2018 •

edited

Loading

felipedf Jul 30, 2018 •

edited

Loading

himdel Jul 30, 2018 •

edited

Loading

felipedf Jul 30, 2018 •

edited

Loading