Adds icon-status component #815

AllenBW · 2017-06-09T21:00:54Z

https://www.pivotaltracker.com/story/show/146029083

TL;DR

adds a new reusable component called icon-status, give it a status and array of possible values for different statuses, and you got ICONS 👶
employs aforementioned component in ansible details component

Takes 5 parameters ; and the icon class name pulled from here

status - string of the current status
success - array of strings for which to display the success icon; pficon pficon-ok
error - array of strings for which to display the error icon; pficon pficon-error-circle-o
queued - array of strings for which to display the circle clock icon; fa fa-clock-o
inprogress - array of strings for which to display the in progress spinner; spinner spinner-xs spinner-inline

If none of the following inputs match the status, the unknown icon will be displayed; pficon pficon-help

Examples of icons used

The unknown catchall

String values are still shown to user

Loicavenel · 2017-06-09T21:13:35Z

@AllenBW any possible screenshots here? I would like to see it

AllenBW · 2017-06-09T21:20:08Z

@Loicavenel yeah when we're outta [wip] there will be all da goodies 👍 🌮

but if ya were wondering what the icons look like...

AllenBW · 2017-06-12T18:53:06Z

@Loicavenel outta wip! GO GO GO GO GO 💃

chriskacerguis · 2017-06-12T21:16:29Z

A few thoughts...

The clock icon appears to be a different size than the PF icons.
The alignment of the icons vs. the "Status" label seem "off"

@serenamarie125 thoughts on those?

(see attached screenshot)

chriskacerguis · 2017-06-12T21:17:36Z

client/app/services/service-details/service-details-ansible.html

@@ -12,8 +12,8 @@ <h3 translate>Results</h3>
                  <div class="form-group">
                    <label class="control-label col-sm-3" translate>Status</label>
                    <div class="col-sm-8">
-                      <input class="form-control text-capitalize" readonly
-                             value="{{item.stack.status || ('Unknown' | translate)}}"/>
+                      <icon-status status="item.stack.status" success="['successful', 'succeeded', 'create_complete']"


Do we want to define the stings here? That could become a VERY long list.

Presently not a long list, hence the deliberate inclusion for this first run. If it gets much longer tho you're spot on, const in the js is gonna be da way to play

Question about these strings being passed into the component, do we have to worry about those strings being translated and having that ruin the functionality they are meant for?

AllenBW · 2017-06-13T12:24:38Z

@chriskacerguis RIGHT!! icon alignment yo, used all things pitched by pf tho so.... idk :-/ i guess we could add custom class to get em all aligned, but that seems like the bandaid for the wrong problem...

chriskacerguis · 2017-06-14T01:32:02Z

@ManageIQ/core-ux for ya'lls review :)

chriskacerguis · 2017-06-14T15:11:26Z

/cc @Loicavenel

Loicavenel · 2017-06-14T15:33:49Z

Just for my clarification, are these status icon are for when Service is provisioned only? or for any actions done at the service level?

cshinn · 2017-06-14T15:34:48Z

@AllenBW I think there may be a mixup regarding the icons used for queued and inprogress; the descriptions don't match the provided icon classes.

Is it possible for an item to have two different status icons at the same? If not, then I don't think the size issue is severe enough to warrant specific code to fix it unless it causes other items to shift around on the page. I do agree, however, that the size discrepancy in the icons is unfortunate. Your thoughts @serenamarie125?

AllenBW · 2017-06-14T15:46:11Z

good catch, the mixup was pr description deep, eyyyyooo @cshinn swapped them around 🙇‍♀️

its technically possible for an item to have more than one icon at a time, this is by design, but one would have to deliberately desire this behavior (and code towards it) most of the time and in this pr, one icon per status. does not affect any other items in page, just looks a lil off when ya line em all up 😆

@Loicavenel icons can be used anywhere, application of this pr is an ansible service.

serenamarie125 · 2017-06-14T16:52:24Z

happened again. I swear i must not hit "COMMENT" after commenting.
THANK YOU @AllenBW for doing this - this is great and will ensure more consistency. Would be assume if this could be shared with Ops UI too :D

Couple of comments

Regarding size, this happens often with FA icons. Often we will end up duplicating them in PF to be sure size/spacing is appropriate. Let's see if we stick with the clock, because i don't think it's right.
success - perfect, this is PF approved ✅
error - perfect, this is PF approved ✅
queued - array of strings for which to display the circle clock icon; fa fa-clock-o❌ let me get back to you be COB
inprogress - array of strings for which to display the in progress spinner; spinner spinner-xs spinner-inline ❓ let me get back to you by COB
unknown icon will be displayed; pficon pficon-help ❌ let me get back to you by COB

HTH!

chriskacerguis · 2017-06-15T14:42:14Z

@serenamarie125 any update for us?

/cc @Loicavenel

serenamarie125 · 2017-06-16T18:51:45Z

@chriskacerguis the current update is that PatternFly does not have appropriate icons at this point. IMO, we have 2 options
1 - use temp icons and open up another issue when icons are available
2 - wait for PF icons to be created ( could be 2+ weeks )

AllenBW · 2017-06-16T18:55:54Z

@serenamarie125 i vote temp! :P

chriskacerguis · 2017-06-16T18:57:08Z

@AllenBW temp will work...I think it would be a better UX with PF icons...@serenamarie125 would you be able to give a more definitive timeline?

serenamarie125 · 2017-06-16T20:04:49Z

Based on the fact that PF does not currently have standards for queued, in progress & unknown, I'd like to suggest one of 2 things:
1 - wait for 2-3 weeks for PF to create these icons
2 - merge as is, but update the icons as soon as PF creates them

If #2, i think we should pick a different icon for queued, because i don't think that one makes sense. Let me know your thoughts @chriskacerguis @Loicavenel

miq-bot · 2017-06-17T15:28:07Z

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

serenamarie125 · 2017-06-17T15:29:10Z

@AllenBW can you do me a favor and explain what the Queued status reflects in this instance? Is it the same as pending ? my concern with the clock icon is that users may confuse it with "scheduled"

serenamarie125 · 2017-06-17T15:30:29Z

@chriskacerguis is 2-3 weeks enough of a timeline? It will be added to our sprint planning, and our 2 week sprint will begin on 6/27 so I anticipate it being completed that sprint.

chriskacerguis · 2017-06-17T15:31:12Z

@serenamarie125 that works great, I think that it would be a better experience to have matching icons.

serenamarie125

Can you change this PR to use fa-hourglass-half for queued? And I will create a new Issue and link to the JIRA story about the PF suggested icons

AllenBW · 2017-06-19T12:31:32Z

@serenamarie125 updated! 🎖 ⚓️

serenamarie125

@AllenBW would be nice if screenshots were updated to reflect code :)

Thanks for making change to hourglass, i 👀 it in the code!

Takes 5 parameteres - status - string of the current status - success - array of strings for which to display the success icon - error - array of strings for which to display the error icon - queued - array of strings for which to display the circle clock icon - inprogress - array of strings for which to display the in progress spinner If none of the following inputs match the status, the unknown icon will be displayed

Supporting succeeded/successful strings for green icon And failed/failure strings for red icon

https://github.com/ManageIQ/manageiq/blob/master/app/models/orchestration_stack/status.rb#L40

miq-bot · 2017-06-19T15:58:16Z

Checked commits AllenBW/manageiq-ui-service@655a7ff~...eec8d3c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

chalettu

@AllenBW , see below for the review comments I made.

chalettu · 2017-06-19T17:32:59Z

client/app/shared/icon-status.component.js

+    inprogress: '<?',
+  },
+  template: `
+   <i class="pficon pficon-ok" ng-if="vm.isSuccess"


I think you could make this template a lot more concise. I would make one icon and write a function in the component that returns the CSS class to show and just use ng-class to display the resulting css class.

Intentionally verbose, favoring readability rather than conciseness, as was demonstrated above, likely these values are going to change

One below, don't have to worry about translations breaking functionality value and check value(s) are passed by the user, most often specified by the backend for both

chalettu · 2017-06-19T17:34:34Z

client/app/services/service-details/service-details-ansible.html

@@ -12,8 +12,8 @@ <h3 translate>Results</h3>
                  <div class="form-group">
                    <label class="control-label col-sm-3" translate>Status</label>
                    <div class="col-sm-8">
-                      <input class="form-control text-capitalize" readonly
-                             value="{{item.stack.status || ('Unknown' | translate)}}"/>
+                      <icon-status status="item.stack.status" success="['successful', 'succeeded', 'create_complete']"


Question about these strings being passed into the component, do we have to worry about those strings being translated and having that ruin the functionality they are meant for?

AllenBW changed the title ~~Adds icon-status component~~ [WIP] Adds icon-status component Jun 9, 2017

AllenBW added the wip label Jun 9, 2017

AllenBW added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 9, 2017

AllenBW force-pushed the PT/#146029083-status-icon-component branch from 6d4f636 to d0c4755 Compare June 12, 2017 16:56

AllenBW added enhancement fine/no and removed wip labels Jun 12, 2017

AllenBW changed the title ~~[WIP] Adds icon-status component~~ Adds icon-status component Jun 12, 2017

AllenBW requested review from chalettu and chriskacerguis June 12, 2017 18:52

chriskacerguis self-assigned this Jun 12, 2017

chriskacerguis requested a review from serenamarie125 June 12, 2017 21:16

chriskacerguis added the ux/review label Jun 12, 2017

chriskacerguis reviewed Jun 12, 2017

View reviewed changes

chriskacerguis previously approved these changes Jun 14, 2017

View reviewed changes

miq-bot added the unmergeable label Jun 17, 2017

serenamarie125 suggested changes Jun 17, 2017

View reviewed changes

AllenBW dismissed chriskacerguis’s stale review via 403e4d1 June 19, 2017 12:31

serenamarie125 approved these changes Jun 19, 2017

View reviewed changes

AllenBW and others added 5 commits June 19, 2017 10:46

Normalize status and status values, fix typos

b4a3a07

Adds icon-status to ansible details service view

8cf4d86

Supporting succeeded/successful strings for green icon And failed/failure strings for red icon

Adds support for normalized and raw orchestation_stack statuses

4e4e021

https://github.com/ManageIQ/manageiq/blob/master/app/models/orchestration_stack/status.rb#L40

Adds icon-status spec [Fixes #146029083]

2ea16fe

AllenBW force-pushed the PT/#146029083-status-icon-component branch from 403e4d1 to ff26362 Compare June 19, 2017 14:47

miq-bot removed the unmergeable label Jun 19, 2017

Update queued status icon

eec8d3c

AllenBW force-pushed the PT/#146029083-status-icon-component branch from ff26362 to eec8d3c Compare June 19, 2017 15:56

chriskacerguis approved these changes Jun 19, 2017

View reviewed changes

chriskacerguis merged commit 474ee8e into ManageIQ:master Jun 19, 2017

chalettu reviewed Jun 19, 2017

View reviewed changes

AllenBW deleted the PT/#146029083-status-icon-component branch June 21, 2017 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds icon-status component #815

Adds icon-status component #815

AllenBW commented Jun 9, 2017 •

edited

Loading

Loicavenel commented Jun 9, 2017

AllenBW commented Jun 9, 2017 •

edited

Loading

AllenBW commented Jun 12, 2017

chriskacerguis commented Jun 12, 2017

chriskacerguis Jun 12, 2017

AllenBW Jun 13, 2017

chalettu Jun 19, 2017

AllenBW commented Jun 13, 2017

chriskacerguis commented Jun 14, 2017

chriskacerguis commented Jun 14, 2017

Loicavenel commented Jun 14, 2017

cshinn commented Jun 14, 2017 •

edited

Loading

AllenBW commented Jun 14, 2017

serenamarie125 commented Jun 14, 2017

chriskacerguis commented Jun 15, 2017

serenamarie125 commented Jun 16, 2017

AllenBW commented Jun 16, 2017

chriskacerguis commented Jun 16, 2017

serenamarie125 commented Jun 16, 2017

miq-bot commented Jun 17, 2017

serenamarie125 commented Jun 17, 2017

serenamarie125 commented Jun 17, 2017

chriskacerguis commented Jun 17, 2017

serenamarie125 left a comment

AllenBW commented Jun 19, 2017

serenamarie125 left a comment

miq-bot commented Jun 19, 2017

chalettu left a comment

chalettu Jun 19, 2017

AllenBW Jun 19, 2017

chalettu Jun 19, 2017

Adds icon-status component #815

Adds icon-status component #815

Conversation

AllenBW commented Jun 9, 2017 • edited Loading

Examples of icons used

The unknown catchall

String values are still shown to user

Loicavenel commented Jun 9, 2017

AllenBW commented Jun 9, 2017 • edited Loading

AllenBW commented Jun 12, 2017

chriskacerguis commented Jun 12, 2017

chriskacerguis Jun 12, 2017

Choose a reason for hiding this comment

AllenBW Jun 13, 2017

Choose a reason for hiding this comment

chalettu Jun 19, 2017

Choose a reason for hiding this comment

AllenBW commented Jun 13, 2017

chriskacerguis commented Jun 14, 2017

chriskacerguis commented Jun 14, 2017

Loicavenel commented Jun 14, 2017

cshinn commented Jun 14, 2017 • edited Loading

AllenBW commented Jun 14, 2017

serenamarie125 commented Jun 14, 2017

chriskacerguis commented Jun 15, 2017

serenamarie125 commented Jun 16, 2017

AllenBW commented Jun 16, 2017

chriskacerguis commented Jun 16, 2017

serenamarie125 commented Jun 16, 2017

miq-bot commented Jun 17, 2017

serenamarie125 commented Jun 17, 2017

serenamarie125 commented Jun 17, 2017

chriskacerguis commented Jun 17, 2017

serenamarie125 left a comment

Choose a reason for hiding this comment

AllenBW commented Jun 19, 2017

serenamarie125 left a comment

Choose a reason for hiding this comment

miq-bot commented Jun 19, 2017

chalettu left a comment

Choose a reason for hiding this comment

chalettu Jun 19, 2017

Choose a reason for hiding this comment

AllenBW Jun 19, 2017

Choose a reason for hiding this comment

chalettu Jun 19, 2017

Choose a reason for hiding this comment

AllenBW commented Jun 9, 2017 •

edited

Loading

AllenBW commented Jun 9, 2017 •

edited

Loading

cshinn commented Jun 14, 2017 •

edited

Loading