-
Notifications
You must be signed in to change notification settings - Fork 357
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
Display providers' suspended status in their quadicon #4022
Display providers' suspended status in their quadicon #4022
Conversation
This pull request is not mergeable. Please rebase and repush. |
app/helpers/quadicon_helper.rb
Outdated
when "valid" | ||
{:fileicon => '100/checkmark.png'} | ||
when "None" | ||
{:fileicon => '100/unknown.png'} |
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 Let's update this to pficon-unknown in a follow-up PR
5730840
to
7f16269
Compare
7f16269
to
1d1750b
Compare
@skateman I approve. |
1d1750b
to
576fd16
Compare
app/helpers/quadicon_helper.rb
Outdated
unless enabled | ||
return { | ||
:fonticon => 'pficon pficon-asleep', | ||
:tooltip => _('This provider is paused, no data is currently collected from it') |
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.
Based on what is described in the issue @slemrmartin created, I feel like the tooltip message is incorrect:
What does it mean - suspend provider? What features should be disabled?
- Ems Refresh
- SmartState analysis of: Vm, Template, Host, ..(?)
- Metrics Collection (C&U)
It's not the whole provider that is suspended, but the data collection.
Correct me if I'm wrong, @slemrmartin.
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.
Yes, you're right, but the second part of tooltip describes what it means, maybe there can be replaced ',' for '-' or '->' for make it more obvious.
Important is, unify the words "paused" / "suspended" according to button names in toolbars
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.
@slemrmartin @skateman I'd suggest something like "Data collection for this provider is paused".
/cc @epwinchell
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.
@romanblanco "Data collection for this provider is suspended" sounds good.
This pull request is not mergeable. Please rebase and repush. |
576fd16
to
09fcd60
Compare
09fcd60
to
6e45fed
Compare
6e45fed
to
b0202ec
Compare
Checked commits skateman/manageiq-ui-classic@3cd7dee~...b0202ec with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
This is actually not depending on any core PR as containers already have this feature. |
After consulting with UX we decided to use the bottom-right quadrant of each provider quadicon to display the suspended status. For now this quadrant is being used to display the authentication status, however, when suspended this information is meaningless so we can safely reuse the same quadrant.
There's some inconsistency in container provider quads that would cause redundant displaying the same information in multiple quadrans, but that's being addressed in a separate PR.
First I had to generalize the
QuadiconHelper.status_img
method to return a hash instead of a string. This way it is possible to handle both fonticons and fileicons simultaneously. Then I added the condition for displaying the suspended icon (without a background color as @epwinchell suggested) and finally set the tooltips.Before:
After:
Related issue: ManageIQ/manageiq#17489
Depends on: ManageIQ/manageiq#17452
@miq-bot add_label pending core, gaprindashvili/no, GTLs
@miq-bot add_reviewer @romanblanco
@miq-bot add_reviewer @epwinchell