Do not map vm status off to status ok #340

yaacov · 2019-04-10T13:29:54Z

Currently we map [VM_STATUS_OFF, VM_STATUS_UNKNOWN] to STATUS_RESULT_OK.

This PR adds mapping for missing statuses to off and warning icon states.

Ref: https://jira.coreos.com/browse/CNV-1466

Screenshot:

coveralls · 2019-04-10T13:34:08Z

Pull Request Test Coverage Report for Build 1133

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 87.322%

Totals
Change from base Build 1342:	0.002%
Covered Lines:	3217
Relevant Lines:	3531

💛 - Coveralls

coveralls · 2019-04-10T13:34:08Z

Pull Request Test Coverage Report for Build 1161

11 of 11 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 87.345%

Totals
Change from base Build 1157:	0.01%
Covered Lines:	3262
Relevant Lines:	3580

💛 - Coveralls

yaacov · 2019-04-10T13:42:36Z

@suomiy @rawagner hi,
we need to map machine down to something other than ok.

We currently have this statuses:
https://github.com/kubevirt/web-ui-components/blob/master/src/components/Dashboard/Inventory/InventoryItemStatus.js#L47
https://github.com/kubevirt/web-ui-components/blob/master/src/components/Dashboard/Inventory/utils.js#L9

Do you think warning is the right option ? should it be error ? should we create a 5'th status icon for off ? do we have state off somewhere else, do we have a mockup for off state ?

atiratree · 2019-04-10T13:55:34Z

Do you think warning is the right option ? should it be error ? should we create a 5'th status icon for off ? do we have state off somewhere else, do we have a mockup for off state ?

VM_STATUS_OFF as a warning doesn't make sense to me. I vote for 5th down state. (Better to ask designers though)

VM_STATUS_OTHER will never be resolved because it is used only as a helper status in getSimpleVmStatus so please remove it. Could you please also rename it to VM_SIMPLE_STATUS_OTHER ? so it is not that confusing

VM_STATUS_UNKNOWN makes sense as a warning

atiratree · 2019-04-10T13:56:53Z

can you do a bit more renaming?

VM_STATUS_ALL -> VM_SIMPLE_STATUS_ALL
VM_STATUS_TO_TEXT -> VM_SIMPLE_STATUS_TO_TEXT

this needs to be also changed in web-ui

yaacov · 2019-04-10T14:33:31Z

@matthewcarleton hi,

We have vm state off, but we do not have an inventory item state off, shoudl we add it ?

Attached is a demo of inventory state off for VMs:

Does this make sense to you ?

lizsurette · 2019-04-10T14:38:34Z

We have vm state off, but we do not have an inventory item state off, shoudl we add it ?

FYI @andybraren

yaacov · 2019-04-10T14:40:19Z

VM_STATUS_ALL -> VM_SIMPLE_STATUS_ALL
VM_STATUS_TO_TEXT -> VM_SIMPLE_STATUS_TO_TEXT

this needs to be also changed in web-ui

@suomiy I will do this a 2 new PR's that we can merge the same time in web-ui and web-ui-components, does that make sense to you ?

src/components/Dashboard/Inventory/InventoryItemStatus.js

atiratree · 2019-04-10T14:58:17Z

@suomiy I will do this a 2 new PR's that we can merge the same time in web-ui and web-ui-components, does that make sense to you ?

It is up to you. I would put it here as it is a small change and a bit relevant to this PR.

atiratree · 2019-04-10T15:13:34Z

+ also VM_STATUS_OTHER -> VM_SIMPLE_STATUS_OTHER

andybraren · 2019-04-10T16:47:39Z

@lizsurette @yaacov @suomiy

Yes, we should have an off state for VMs within the Inventory card:
VM_STATUS_OFF -> Off icon (as your screenshot shows)

We'd like for the Inventory card's statuses to be as close as possible to what a user would see if they looked at the VM List page. For that reason, I would prefer to use the same status icon in the Inventory card that VM_STATUS_UNKNOWN shows in the VM List page. If we currently use pficon-warning-triangle-o then that's what we should use in the Inventory card. If we use pficon-unknown then we should use that instead.

yaacov · 2019-04-11T05:42:42Z

I added the new inventory "off" state looking like the vm "off", only bigger because all the other icons in inventory are big.

@suomiy @rawagner
I tried to use "x2" in the "off" icon but it did not make the icon bigger ( used special class css instead ) am I missing something with the "size" on pficon ?

yaacov · 2019-04-11T05:47:45Z

@andybraren hi,
do we need some pop up text to appear when we hover over the status icons ?

for example the word "Off" when hovering over the "off" icon ? @suomiy @rawagner ?

atiratree · 2019-04-11T09:49:20Z

We'd like for the Inventory card's statuses to be as close as possible to what a user would see if they looked at the VM List page. For that reason, I would prefer to use the same status icon in the Inventory card that VM_STATUS_UNKNOWN shows in the VM List page. If we currently use pficon-warning-triangle-o then that's what we should use in the Inventory card. If we use pficon-unknown then we should use that instead.

There is not so much space to show an icon for every state we have, like VM_STATUS_UNKNOWN. I think it makes more sense to show a specific set of generic set of icons (VM_STATUS_OFF should be probably one of them). Then the user can go to the VMs page to inspect what the warnings, etc. mean.

atiratree · 2019-04-11T09:50:22Z

for example the word "Off" when hovering over the "off" icon ? @suomiy @rawagner ?

My vote is that it is not necessary.

yaacov · 2019-04-11T09:53:05Z

@suomiy is there a "correct" way to make the off icon size bigger ?

yaacov · 2019-04-11T09:53:28Z

@suomiy @rawagner I think it's ready, please review.

atiratree · 2019-04-11T10:08:35Z

@suomiy is there a "correct" way to make the off icon size bigger ?

patternfly icon is just a switch between PatternflyIcon and FontAwesomeIcon components.

Only the FontAwesomeIcon component supports size property, so your only option after that is the css.

Can you please also remove the size property on inProgress icon and apply the css?

yaacov · 2019-04-11T10:11:23Z

Can you please also remove the size property on inProgress icon and apply the css?

Sure 🍽️

atiratree · 2019-04-11T10:09:44Z

sass/components/Dashboard/inventory.scss

@@ -43,3 +43,7 @@
 .kubevirt-inventory__row-status-item-icon--warn {
  color: goldenrod;
 }
+
+.kubevirt-inventory__row-status-item-icon--off {
+  font-size: 1.5em;


I tried it in cosmos and maybe it could be a tad bigger

atiratree · 2019-04-11T10:10:08Z

src/components/Dashboard/Inventory/InventoryItemStatus.js

@@ -42,13 +42,23 @@ const InProgressStatus = ({ count }) => (



InProgressStatus is also type="pf" component. Please try the sizes.

atiratree · 2019-04-11T10:13:06Z

src/components/Dashboard/Inventory/InventoryItemStatus.js

 const Status = ({ Component, count, ...props }) => count > 0 && <Component count={count} {...props} />;

-export const InventoryItemStatus = ({ ok, warn, error, inProgress }) => (
+export const InventoryItemStatus = ({ ok, warn, error, inProgress, off }) => (


please showcase this in the fixture

atiratree · 2019-04-11T10:31:32Z

build failing..

yaacov · 2019-04-11T10:35:15Z

@suomiy I do not understand why I get Ki instead of KiB looking ...

yaacov · 2019-04-11T10:36:40Z

@suomiy not sure we can merge this ... something is wrong with the KiB units, looking what happen :-(

atiratree · 2019-04-11T10:44:56Z

@suomiy I do not understand why I get Ki instead of KiB looking ...

it was merged recently. Please rebase on the recent master

yaacov · 2019-04-11T10:50:14Z

@suomiy I'm rebased on master and get Ki is it the way it suppose to be ?

yaacov · 2019-04-11T10:55:25Z

ahh... #338

@suomiy it's ok, somehow this snap changes were not part of #338 but we do need them.

atiratree · 2019-04-11T10:56:01Z

Yes you have it correct. The build is failing on master.

atiratree · 2019-04-11T10:58:17Z

we merged wrong snapshots before ^^

andybraren · 2019-04-11T13:43:29Z

@suomiy

We'd like for the Inventory card's statuses to be as close as possible to what a user would see if they looked at the VM List page. For that reason, I would prefer to use the same status icon in the Inventory card that VM_STATUS_UNKNOWN shows in the VM List page. If we currently use pficon-warning-triangle-o then that's what we should use in the Inventory card. If we use pficon-unknown then we should use that instead.

There is not so much space to show an icon for every state we have, like VM_STATUS_UNKNOWN. I think it makes more sense to show a specific set of generic set of icons (VM_STATUS_OFF should be probably one of them). Then the user can go to the VMs page to inspect what the warnings, etc. mean.

Acknowledged - this is something we're working on, and likely involves a deeper dive on our end into all of the possible statuses, how to prioritize them, when to show which, etc. In the short term we will very likely propose removing the green "okay" status from the Inventory card entirely (for this and other reasons). We'll share that design proposal soon.

atiratree · 2019-04-11T14:36:55Z

oki

we will very likely propose removing the green "okay" status

Are you okay with showing no status if okay part is removed? Because I am not sure we have much else to show for example in PVCs. Only pending icon when the pvc is pending.

andybraren · 2019-04-12T21:34:52Z

Are you okay with showing no status if okay part is removed? Because I am not sure we have much else to show for example in PVCs. Only pending icon when the pvc is pending.

Yes, that's totally fine. 👍

kubevirt-bot added do-not-merge/work-in-progress size/M labels Apr 10, 2019

atiratree reviewed Apr 10, 2019

View reviewed changes

src/components/Dashboard/Inventory/InventoryItemStatus.js Show resolved Hide resolved

yaacov mentioned this pull request Apr 10, 2019

Use diferent nameing schema for status group than single status kubevirt/web-ui#293

Merged

kubevirt-bot added size/L and removed size/M labels Apr 10, 2019

yaacov changed the title ~~[WIP] Do not map vm status off to status ok~~ Do not map vm status off to status ok Apr 10, 2019

kubevirt-bot removed the do-not-merge/work-in-progress label Apr 10, 2019

yaacov added 5 commits April 11, 2019 12:42

Do not map vm status off to status ok

4eb0b50

Add off inventory status icon

3fc2d36

rename status group consts to diferentiate from status

d6f776f

reorder icons off before ok

2615dcc

Group syntetic statuses together

0236c4d

atiratree suggested changes Apr 11, 2019

View reviewed changes

size using css for icons type pf

61f7b23

yaacov force-pushed the do-not-map-vm_status_off-to-ok branch from 349f0e0 to 61f7b23 Compare April 11, 2019 10:18

status icon resize to look more same size with other icons

1b11c1f

atiratree approved these changes Apr 11, 2019

View reviewed changes

update snaps

d992ede

atiratree merged commit 2306c97 into kubevirt:master Apr 11, 2019

mareklibra mentioned this pull request Apr 15, 2019

Bump 0.1.27 #353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not map vm status off to status ok #340

Do not map vm status off to status ok #340

yaacov commented Apr 10, 2019 •

edited

Loading

coveralls commented Apr 10, 2019

coveralls commented Apr 10, 2019 •

edited

Loading

yaacov commented Apr 10, 2019

atiratree commented Apr 10, 2019

atiratree commented Apr 10, 2019

yaacov commented Apr 10, 2019

lizsurette commented Apr 10, 2019

yaacov commented Apr 10, 2019

atiratree commented Apr 10, 2019

atiratree commented Apr 10, 2019

andybraren commented Apr 10, 2019

yaacov commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree commented Apr 11, 2019

atiratree commented Apr 11, 2019

yaacov commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree Apr 11, 2019

atiratree Apr 11, 2019

atiratree Apr 11, 2019

atiratree commented Apr 11, 2019

yaacov commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree commented Apr 11, 2019

yaacov commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree commented Apr 11, 2019

atiratree commented Apr 11, 2019

andybraren commented Apr 11, 2019

atiratree commented Apr 11, 2019

andybraren commented Apr 12, 2019

		@@ -42,13 +42,23 @@ const InProgressStatus = ({ count }) => (

Do not map vm status off to status ok #340

Do not map vm status off to status ok #340

Conversation

yaacov commented Apr 10, 2019 • edited Loading

coveralls commented Apr 10, 2019

Pull Request Test Coverage Report for Build 1133

💛 - Coveralls

coveralls commented Apr 10, 2019 • edited Loading

Pull Request Test Coverage Report for Build 1161

💛 - Coveralls

yaacov commented Apr 10, 2019

atiratree commented Apr 10, 2019

atiratree commented Apr 10, 2019

yaacov commented Apr 10, 2019

lizsurette commented Apr 10, 2019

yaacov commented Apr 10, 2019

atiratree commented Apr 10, 2019

atiratree commented Apr 10, 2019

andybraren commented Apr 10, 2019

yaacov commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree commented Apr 11, 2019

atiratree commented Apr 11, 2019

yaacov commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree Apr 11, 2019

Choose a reason for hiding this comment

atiratree Apr 11, 2019

Choose a reason for hiding this comment

atiratree Apr 11, 2019

Choose a reason for hiding this comment

atiratree commented Apr 11, 2019

yaacov commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree commented Apr 11, 2019

yaacov commented Apr 11, 2019

yaacov commented Apr 11, 2019

atiratree commented Apr 11, 2019

atiratree commented Apr 11, 2019

andybraren commented Apr 11, 2019

atiratree commented Apr 11, 2019

andybraren commented Apr 12, 2019

yaacov commented Apr 10, 2019 •

edited

Loading

coveralls commented Apr 10, 2019 •

edited

Loading