Support new filtering options on Topology page

[EsdrasVP]

This PR is able to:

• Add support to new filtering options in the Topology page
• Add Health State filter options for Physical Infrastructure Topology page
• Add the possibility to have multiple values to the same filter (as the Unknown/None case)
• Change filters and names font-family to Open Sans

Depends on:
kubernetes-ui/topology-graph#29

[EsdrasVP]

@miq-bot assign @himdel

[himdel]

@EsdrasVP I think this looks good 👍 (I still have to test properly though)

But, looks like the specs need fixing to expect that extra field.. :)

[himdel]

cc @skateman

[EsdrasVP]

@EsdrasVP I think this looks good (I still have to test properly though)

But, looks like the specs need fixing to expect that extra field.. :)

@himdel Thanks for the reminder, fixed them and now they should work fine.

[EsdrasVP]

@miq-bot add_label ux/review

@terezanovotna Could you take a look at it too?

[terezanovotna]

Hi @EsdrasVP,

1. Can we make the icons black? Both in the top area and the topology area. When I look at Physical Chassis that has red icon, it seems like something is wrong with it, it is just not happy way to use color coding in this way.
2. I do not think we need icons for the health state. For ex: Valid is now represented with a checkbox, but that checkbox is not present anywhere in the topology area, and therefore can be confusing for the user. I suggest using just color dots.
3. Are we using SansOpen font for all?

Here is a mockup:

What do you think @EsdrasVP? Let me know if you have any questions.

[himdel]

Except for what @terezanovotna said, LGTM :).

All the existing topologies still work with kubernetes-ui/topology-graph#29 (+ the css changes in here).
The new filters also work 👍

Re icon colors: even the blue icons should be black. (The blue was introduced by openshift at some point and got copied over, it should not be blue in containers either.)

[EsdrasVP]

Hi @terezanovotna, regarding these questions:

1. We can, actually. Initially I thought that, since Racks were green as the state "Valid" and Switches were orange as "Warning", there was no problem in using this color.
2. I've put icons on health state filters so the filter bar could be consistent. Also I didn't separate them as a new category for the same reason, but if it's more legible that's ok for me. I just have to work on this because currenlty there is no division by type in this div.
3. Currenlty no, it's only set as sans-serif, but I changed to Open Sans both filter names and display names.

Is there something else you want to address?

[EsdrasVP]

Re icon colors: even the blue icons should be black. (The blue was introduced by openshift at some point and got copied over, it should not be blue in containers either.)

@terezanovotna @himdel So all components icons should be black?

[terezanovotna]

Yes, all components icons should be black @EsdrasVP

[douglasgabriel]

Hey guys, I would say that we got a good readability of our topology differentiating components also by colors and not only by the icon, so, making all components black we will go back to the chaos that was before, to distinguish kinds of components in a dense topology. In a previous conversation with @terezanovotna we discuss about not use only colors to communicate with the user, once some people has different perceptions of colors, so, to solve this problem, we have different icons, but even so, we also can improve the communication with those people that not have such perception issues. @terezanovotna, @himdel and @EsdrasVP what do you think?

P.S. please don't make all icons black 🙏 😢

[himdel]

I think we want the icons black precisely so that we can use color for state, and not end up with a rainbow cake.

But.. not my decision, staying out of that one. :)

Not that related to this PR anyway, just don't add any red ones here please, those do look like an error indicator.

[EsdrasVP]

I agree with @douglasgabriel. This was already a topic of discussion before and we agreed that assign different colors for each component is more legible for the user. @himdel we could avoid using critical state colors, the problem is that we have few options left available by pattern fly palette.

[himdel]

@EsdrasVP Do you have any links to that discussion?

As far as I know, black icons is the decision that stands.

[douglasgabriel]

here is @himdel #3999

[terezanovotna]

@douglasgabriel @EsdrasVP I had no problem with colored icons before we added Health State. As health state is communicated via colors, we cannot use color coding for two same purposes as it is confusing for the user.

As we show the health state in the colors, it would be great to have the icons black.

[miq-bot]

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

[himdel]

@EsdrasVP looks like as it is now, this is breaking the styling of filters in all the other topologies:

[himdel]

@EsdrasVP now that the change in kubernetes-topology-graph is released, can you please add this to make it use that new release? :)

diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js
index 7826cc9b9..a980426bd 100644
--- a/app/assets/javascripts/application.js
+++ b/app/assets/javascripts/application.js
@@ -32,7 +32,7 @@
 //= require bower_components/d3/d3
 //= require bower_components/c3/c3
 //= require bower_components/lodash/lodash
-//= require bower_components/kubernetes-topology-graph/dist/topology-graph
+//= require kubernetes-topology-graph/dist/topology-graph
 //= require ./miq_browser_detect
 //= require ./miq_application
 //= require ./miq_flash
diff --git a/app/assets/stylesheets/application.css b/app/assets/stylesheets/application.css
index cf860cbae..e735d5251 100644
--- a/app/assets/stylesheets/application.css
+++ b/app/assets/stylesheets/application.css
@@ -11,7 +11,7 @@
  *= require ./miq_tree
  *= require bower_components/codemirror/lib/codemirror
  *= require bower_components/codemirror/theme/eclipse
- *= require bower_components/kubernetes-topology-graph/dist/topology-graph
+ *= require kubernetes-topology-graph/dist/topology-graph
  *= require ./wizard
  *= require ./timeline
  *= require ./topology
diff --git a/bower.json b/bower.json
index f519d06d2..d01339ab6 100644
--- a/bower.json
+++ b/bower.json
@@ -32,7 +32,6 @@
     "jquery-ui": "jqueryui#~1.12.1",
     "jquery-ujs": "~1.2.2",
     "jquery.observe_field": "himdel/jquery.observe_field#~0.1.0",
-    "kubernetes-topology-graph": "~0.0.23",
     "lodash": "~3.10.0",
     "manageiq-ui-components": "bower-dev",
     "moment-duration-format": "~1.3.0",
diff --git a/package.json b/package.json
index 81b8b8486..f2f6f61b3 100644
--- a/package.json
+++ b/package.json
@@ -35,6 +35,7 @@
     "graphql": "~0.12.0",
     "history": "^4.7.2",
     "jquery": "~2.2.4",
+    "kubernetes-topology-graph": "~0.0.25",
     "lodash": "^4.17.4",
     "patternfly": "~3.54.1",
     "patternfly-react": "2.10.1",

(referencing #3734 because this moves the graph to npm)

[EsdrasVP]

@EsdrasVP looks like as it is now, this is breaking the styling of filters in all the other topologies:

@himdel I changed the padding to see if it fixes the problem. Could you check that?

[himdel]

@EsdrasVP all the topologies look consistent now, thanks! :)

[himdel]

@EsdrasVP maybe 2 little things you could fix?

• .filter-group - that's a bit too generic a name, can you use topology-filter-group or something similar?
• .topology .legend label has cursor: pointer - that's fine, but it now means the "Health state:" label also has cursor: pointer, which is misleading as it's not really clickable. Can you adjust so that "Health state" uses the proper cursor?
• and we can't merge without Support new filtering options on Topology page #4396 (comment)

[EsdrasVP]

@EsdrasVP maybe 2 little things you could fix?

.filter-group - that's a bit too generic a name, can you use topology-filter-group or something similar?
.topology .legend label has cursor: pointer - that's fine, but it now means the "Health state:" label also has cursor: pointer, which is misleading as it's not really clickable. Can you adjust so that "Health state" uses the proper cursor?
and we can't merge without #4396 (comment)

@himdel That's fine, I changed the .filter-group to .topology-filter-group and added the cursor: pointer to the Health State label. Also, I already made the modifications required on #4396 (comment).

[himdel]

@himdel That's fine, I changed the .filter-group to .topology-filter-group and added the cursor: pointer to the Health State label. Also, I already made the modifications required on #4396 (comment).

@EsdrasVP Perfect, thanks!

Except... if that's so, you may need to git push ;)

[EsdrasVP]

@EsdrasVP Perfect, thanks!

Except... if that's so, you may need to git push ;)

@himdel I already did, and I closed and reopened the PR in order to Travis run correctly.

[himdel]

@EsdrasVP please look at the files changed screen.. There is no change to package.json, and I still see css for filter-group.

[EsdrasVP]

@EsdrasVP please look at the files changed screen.. There is no change to package.json, and I still see css for filter-group.

@himdel Sorry for that, now the modifications are here.

[miq-bot]

Checked commit EsdrasVP@c8252a7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. 👍

[himdel]

@EsdrasVP no problem, now I can see it all :) aand it works, without breaking the other topologiges 👍 :) Thanks :)