Quadicon improvements #3046

karelhala · 2017-12-13T11:47:13Z

Introduces

New method to decorators

This PR adds new method to decorators which is responsible for generating quadicons. This feature is used only for ExtManagementSystem providers:

`CloudManager`

Before

After

`ContainerManager`

Before

After

`DataWarehouseManager`

Not a provider, but since it inherits from ExtManagementSystem it can generate quadicon

`InfraManager`

Before

After

`MiddlewareManager`

Before

After

`MonitoringManager`

Not a provider, but since it inherits from ExtManagementSystem it can generate quadicon

`NetworkManager`

Before

After

`PhysicalInfraManager`

New methods to quadion helper

New methods inside quadicon helper furthermore simplifies generation of quadicon

render_quadicon_text - for rendering text inside quadion, every text with more than 3 characters will use smaller font
render_quad_image - for rendering images and fonticons, it will use either flobj_img_small or flobj_img_simple based on type provided in quad descritption
transform_quadicon - method for transforming quadicon hash to array of strings with location where these strings should be rendered.

GH issue #3036

This PR follows GH issue #3036 to implement correctly format which is produced by quadicon method.

What to look out for

Font size for any quad has been unified with top left. Only top left had change on font if length of string was bigger than 2, so this PR unifies it that any text in quad longer than 2 chars has this font change.

UI changes

Adds new fields to quadicon for MW provider

Domains	Servers
Icon	Auth

karelhala · 2017-12-13T11:47:48Z

@miq-bot add_label gaprindashvili/no, enhancement
@skateman please review and test

skateman

The AssetsMapper is in general a confusing name, can't you just omit it?

skateman · 2017-12-13T11:54:37Z

app/helpers/quadicon_helper/assets_mapper.rb

@@ -0,0 +1,14 @@
+module QuadiconHelper
+  class AssetsMapper


this is a confusing name

also why is this a class?

skateman · 2017-12-13T11:54:48Z

app/helpers/quadicon_helper/assets_mapper.rb

+module QuadiconHelper
+  class AssetsMapper
+    class << self
+      def img_status(item)


this is an even more confusing name inside

skateman · 2017-12-13T11:55:03Z

app/helpers/quadicon_helper.rb

-      output << flobj_img_simple("svg/vendor-#{h(item.image_name)}.svg", "c72")
-      output << flobj_img_simple(img_for_auth_status(item), "d72")
+      top_left = quadicon[:top_left][:text]
+      top_right = quadicon[:top_right][:text]


martinpovolny · 2017-12-13T12:37:15Z

@karelhala : I think that this is exactly the direction we need to folllow with quadicons. It attacks the technical debt, allows pluggability and also makes a step towards the client side (client side quadicon rendering).

I'd love to get a very good consensus on this.

@AparnaKarve: would you like to participate in this effort?

abonas · 2017-12-13T14:45:21Z

what providers are the datawarehouse and monitoring manager? do they use quadicons?

abonas · 2017-12-13T14:41:15Z

app/decorators/manageiq/providers/cloud_manager_decorator.rb

+  class CloudManagerDecorator < MiqDecorator
+    def quadicon(_n = nil)
+      {
+        :top_left     => { :text => container_nodes.size },


why container nodes are in the cloud manager?

abonas · 2017-12-13T14:41:48Z

app/decorators/ext_management_system_decorator.rb

+
+  def quadicon(_n = nil)
+    {
+      :top_left     => { :text => try(:hosts).try(:size) || 0 },


isn't the ext_mgmt the top level? does it have hosts as an entity? or it's a default fallback?

This is just deafult fallback.

abonas · 2017-12-13T14:42:46Z

app/decorators/manageiq/providers/hawkular/middleware_manager_decorator.rb

+          :fileicon => fileicon,
+          :tooltip  => type
+        },
+        :bottom_right => {


can the bottom right part = auth status be extracted as a default? it seems repeating

@abonas I agree with you as I'm a fan of the DRY philosophy, but decorators are an exception. Think about them as a place to specify the UI sugar for each DB model. Eventually it will get simplified to a DSL that can be easily edited by designers:

class SomethingDecorator quadicon :top_left, :fileicon => '100/something.png', :tooltip => 'yay' quadicon :top_right, :text => 3 end

I think it's understandable that we don't want to pull shared definitions on such places...

abonas · 2017-12-13T14:43:25Z

app/decorators/manageiq/providers/kubernetes/container_manager_decorator.rb

+
+    def quadicon(_n = nil)
+      {
+        :top_left     => { :text => total_vms },


why are there vms in container manager quadicon?

abonas · 2017-12-13T14:46:06Z

app/helpers/quadicon_helper.rb

-    when "None"    then "100/unknown.png"
-    else                "100/exclamationpoint.png"
-    end
+    status_img(item)


why does this belong to refactoring of the quadicons?

Well we need to return url for item's status, and it is better to move these to seperate file which is imported and is easier to read.

@karelhala : do we really need the URL?

I mean we figured when working on the GTLs that the URLs are calculated on 2 places and that really matters is the single URL that is actually not part of the quadicon.

What about removing all the URL logic here? Instead of refactoring that part. That might actually save some time and code.

@martinpovolny this does not touches urls, these are just paths to images.

wanted to say the same 😉

abonas · 2017-12-13T14:46:26Z

app/helpers/quadicon_helper.rb

@@ -224,7 +221,7 @@ def img_tag_reflection
  end

  # Renders a quadicon for PhysicalServer
-  #
+  # TODO: use quadicon decorator


what about all the todos? is it for followup PR? to be fixed by provider teams?

It's to mark methods which can use quadicon decorator. I can refactor them now, or in follow up PR. Decision is on @martinpovolny or @skateman.

Marking the methods is ok. We cannot do all cleanups at once. Thx!

abonas · 2017-12-13T15:44:27Z

I am also concerned this refactors all providers without proper checks available to ensure there are no regressions. Afaik there are no tests and I assume that there wont be a real env check for all providers by @karelhala

abonas · 2017-12-13T18:41:18Z

app/decorators/manageiq/providers/kubernetes/container_manager_decorator.rb

+    def quadicon(_n = nil)
+      {
+        :top_left     => { :text => container_nodes.size },
+        :top_right    => { :text => enabled? ? "on" : "paused" },


Where is the on/paused taken from? I don't see the original code from before the refactoring, so I wonder...

This comes from app/helpers/quadicon_helper.rb:571. However this is incorrect because it should be icon and not text.

abonas · 2017-12-13T18:42:55Z

app/helpers/quadicon_helper/decorator.rb

+
+  def status_img(item = nil)
+    case item.authentication_status
+    when "Invalid" then "100/x.png"


can the "100" be not repeated/gotten from somewhere else? it might be changed tomorrow to 150 :) and this hardcoding will cause a regression

This is question probably for @skateman since it's related to assets pipeline and it used to be in this exact format.

@abonas we have the 100 everywhere and we want to keep it consistent. Eventually, we're going to replace all these things with fonticons (that aren't yet supported by quadicons) and SVGs (in progress).

abonas · 2017-12-13T18:44:03Z

app/decorators/manageiq/providers/infra_manager_decorator.rb

+    def quadicon(_n = nil)
+      {
+        :top_left     => { :text => hosts.size },
+        :top_right    => { :text => total_vms },


I don't see the original code, so I am wondering why hosts.size is called, and then total_vms (without the call to size method) ? does it work in openstack provider too where there are instances? or they are also called vms in miq?

Well, this comes from app/helpers/quadicon_helper.rb:570 and EmsInfra is alias to ManageIQ::Providers::InfraManager

abonas · 2017-12-13T18:46:10Z

what providers are the datawarehouse and monitoring manager? do they use quadicons? I am not sure they are providers at all

karelhala · 2017-12-14T12:26:35Z

what providers are the datawarehouse and monitoring manager? do they use quadicons? I am not sure they are providers at all

They inherit from ext_management_system.rb and since we have decorator for this manager it automatically enables generation of quadicons for any manager which inherits from ExtManagementSystem (or BaseManager). However that does not mean they use quadicons.

karelhala · 2017-12-14T13:18:17Z

All providers checked and they look same with this PR, except PhysicalInfraManager since I don't have any provider can you @AparnaKarve please check it?

abonas · 2017-12-14T13:35:56Z

app/decorators/manageiq/providers/container_manager_decorator.rb

+    def quadicon(_n = nil)
+      {
+        :top_left     => {:text => container_nodes.size},
+        :top_right    => {:state_icon => enabled? ? "on" : "paused"},


where is the on/paused info taken from?

#3046 (comment) it's from refactored code quadicon_helper.rb:571

AparnaKarve · 2017-12-14T20:50:05Z

All providers checked and they look same with this PR, except PhysicalInfraManager since I don't have any provider can you @AparnaKarve please check it?

@karelhala The Before and After for PhysicalInfraManager look the same.

epwinchell · 2017-12-15T04:23:33Z

@karelhala Something went wrong with the font size on the 3-digit "after"

karelhala · 2017-12-15T08:38:23Z

Something went wrong with the font size on the 3-digit "after"

That's actually wanted. Before only top left quad with 3+ texts had smaller font size, so I unified it accross all quads. If you want I can get rid of it completely.

karelhala · 2017-12-15T08:40:08Z

Restarting travis.

… infra and container

miq-bot · 2017-12-15T14:15:18Z

Checked commits karelhala/manageiq-ui-classic@6a7c018~...fe44388 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
11 files checked, 0 offenses detected
Everything looks fine. ⭐

karelhala · 2018-01-11T09:52:34Z

@martinpovolny can we merge this? Or are we waiting with this untill G release?

miq-bot added enhancement gaprindashvili/no labels Dec 13, 2017

skateman suggested changes Dec 13, 2017

View reviewed changes

karelhala force-pushed the quadionImprovements branch from 86b821b to 92e137b Compare December 13, 2017 12:27

karelhala changed the title ~~Quadion improvements~~ Quadixon improvements Dec 13, 2017

karelhala changed the title ~~Quadixon improvements~~ Quadicon improvements Dec 13, 2017

karelhala force-pushed the quadionImprovements branch 4 times, most recently from fcca3a8 to 8a42d62 Compare December 13, 2017 14:30

abonas suggested changes Dec 13, 2017

View reviewed changes

karelhala force-pushed the quadionImprovements branch 4 times, most recently from 76a34fd to 21574e8 Compare December 13, 2017 15:31

karelhala force-pushed the quadionImprovements branch 4 times, most recently from 0377160 to fb32b42 Compare December 13, 2017 16:20

abonas reviewed Dec 13, 2017

View reviewed changes

karelhala force-pushed the quadionImprovements branch from fb32b42 to 1a3a27a Compare December 14, 2017 13:16

abonas reviewed Dec 14, 2017

View reviewed changes

karelhala force-pushed the quadionImprovements branch from 1a3a27a to 93cac0d Compare December 14, 2017 16:03

karelhala closed this Dec 15, 2017

karelhala reopened this Dec 15, 2017

karelhala added 2 commits December 15, 2017 14:46

Move quadicon definition to decorator and use it for MW manager

6a7c018

Enable quadicon decorator for Ext systems and use specific for cloud,…

d7ae04b

… infra and container

karelhala force-pushed the quadionImprovements branch from 93cac0d to e1f241e Compare December 15, 2017 13:46

Use new methods render text and image in quadicon

fe44388

karelhala force-pushed the quadionImprovements branch from e1f241e to fe44388 Compare December 15, 2017 14:05

karelhala mentioned this pull request Dec 15, 2017

[Tracking] Quadicon rendered on frontend #3076

Closed

4 tasks

martinpovolny added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 19, 2018

martinpovolny merged commit 15cf2f3 into ManageIQ:master Jan 19, 2018

Quadicon improvements #3046

Quadicon improvements #3046

Conversation

karelhala commented Dec 13, 2017 • edited Loading

Introduces

New method to decorators

CloudManager

Before

After

ContainerManager

Before

After

DataWarehouseManager

InfraManager

Before

After

MiddlewareManager

Before

After

MonitoringManager

NetworkManager

Before

After

PhysicalInfraManager

New methods to quadion helper

GH issue #3036

What to look out for

UI changes

karelhala commented Dec 13, 2017

skateman left a 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

martinpovolny commented Dec 13, 2017

abonas commented Dec 13, 2017

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

skateman Dec 14, 2017 • 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

Choose a reason for hiding this comment

abonas commented Dec 13, 2017

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

skateman Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonas commented Dec 13, 2017

karelhala commented Dec 14, 2017

karelhala commented Dec 14, 2017

Choose a reason for hiding this comment

karelhala Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

AparnaKarve commented Dec 14, 2017

epwinchell commented Dec 15, 2017

karelhala commented Dec 15, 2017

karelhala commented Dec 15, 2017

miq-bot commented Dec 15, 2017

karelhala commented Jan 11, 2018

karelhala commented Dec 13, 2017 •

edited

Loading

`CloudManager`

`ContainerManager`

`DataWarehouseManager`

`InfraManager`

`MiddlewareManager`

`MonitoringManager`

`NetworkManager`

`PhysicalInfraManager`

skateman Dec 14, 2017 •

edited

Loading

skateman Dec 14, 2017 •

edited

Loading

karelhala Dec 14, 2017 •

edited

Loading