-
Notifications
You must be signed in to change notification settings - Fork 356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Missing icon for powering-up state #4101
Conversation
This PR fixes ManageIQ/manageiq-providers-kubevirt#84 |
@himdel Please review |
app/helpers/quadicon_helper.rb
Outdated
@@ -32,6 +32,7 @@ module QuadiconHelper | |||
'terminated' => {:fonticon => 'pficon pficon-off', :background => '#CC0000'}, | |||
'off' => {:fonticon => 'pficon pficon-off', :background => '#CC0000'}, | |||
'template' => {:fonticon => 'pficon pficon-template', :background => '#336699'}, | |||
'powering_up' => {:fileicon => 'svg/currentstate-powering-up.svg', :background => '#F0AB00'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The background image doesn't really make sense with the SVG, only with a fonticon. My suggestion would be fa fa-play
but maybe @epwinchell can tell you a better one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skateman We are using this SVG in vm details view. Do we want to be consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkliczewski consistent with what? @martinpovolny is trying to pull these fonticon+color+background definitions into textual summaries 😉 also the triangle seems as an already deprecated patternfly icon, let's wait for Eric's opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skateman Consistent with vm detail view where this svg is used already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SVG with the triangle is AFAIK deprecated and we went to the pficon-off
+ background color layout from the play/pause/stop/etc icons, at least on quadicons. The summary screens (VM detail) are still using the old icons, but that's just a question of time to change them too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkliczewski @skateman For powering-up, I suggest using "pficon-on " with #FF9900 as the background (orange) The play arrow (fa-play) is no longer to be used since Patternfly developed new power state font-icons.
Likewise, I recommend "pficon-off" with the orange background to represent "powering down" or "stopping"
This will be consistent with the toolbar changes that were approved by UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'powering_up' => {:fonticon => 'pficon pficon-on', :background => '#FF9900'},
@pkliczewski looks good to me, please update the screenshots and I will approve this 🍻 |
There is a vm state - powering-up for which there is no icon in general vm view. This PR fixes this issue by reusing existing svg.
Checked commit pkliczewski@72175a4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a vm state - powering-up for which there is no icon in general vm view.
This PR fixes this issue by reusing existing svg.
Now when a vm is powering-up it looks like this: