Add a tooltip to health state of the cards in the physical infra dashboard #4453

CharlleDaniel · 2018-08-10T17:40:44Z

This pr is able to:

Remove the redundant number if all components are healthy.
Before

After
Add a tooltip in the health state icon in the provider dasborad

CharlleDaniel · 2018-08-10T17:45:04Z

@terezanovotna

CharlleDaniel · 2018-08-10T19:15:28Z

@miq-bot add_label enhancement
@miq-bot add_label gaprindashvili/no
@miq-bot add_label ux/review

mzazrivec · 2018-08-13T09:38:57Z

app/services/ems_physical_infra_dashboard_service.rb

+      if critical_count.positive?
+        icon_class = 'pficon pficon-error-circle-o'
+        count = critical_count
+        description = "#{count} #{component_type.downcase} are in critical state."


Missing gettext.

@mzazrivec done.

mzazrivec · 2018-08-13T09:39:06Z

app/services/ems_physical_infra_dashboard_service.rb

+      elsif warning_count.positive?
+        icon_class = 'pficon pficon-warning-triangle-o'
+        count = warning_count
+        description = "#{count} #{component_type.downcase} may present unexpected behavior."


Missing gettext.

mzazrivec · 2018-08-13T09:39:18Z

app/services/ems_physical_infra_dashboard_service.rb

+        description = "#{count} #{component_type.downcase} may present unexpected behavior."
+      elsif valid_count.positive?
+        icon_class = 'pficon pficon-ok'
+        description = "All #{component_type.downcase} are healthy!"


Missing gettext.

terezanovotna · 2018-08-14T08:07:43Z

LGTM!

@miq-bot remove_label ux/review
@miq-bot add_label ux/approved

mzazrivec · 2018-08-14T08:27:17Z

app/services/ems_physical_infra_dashboard_service.rb

+      if critical_count.positive?
+        icon_class = 'pficon pficon-error-circle-o'
+        count = critical_count
+        description = _("#{count} #{component_type.downcase} are in critical state.")


The above won't work. You need the following:

_("%{count} %{component_type} are in critical state.") % {:count => count, :component_type => component_type.downcase}``` Same below.

Does this thing have a singular/plural form? I see 1 servers are in critical state as a tooltip.

hey @mzazrivec I will change this, but could you explain what won't work ?

@skateman This doesn't have singular/plural, If you want I could add as well.

I got the tooltip, but the number is still there:

@skateman About that, the idea is only to remove it if all components are healthy. If we have any state as warning or critical we should show their count.

@skateman Now was added a singular and plural message. Could you review again?

@mzazrivec Done, the gettext was fixed. Also thank you for your explanation, was very very nice.

@CharlleDaniel for the singular/plural, you can use n_(singular_text, plural_text, number)

@skateman This not worked for me, but thank you for your suggestion.

I think you should use n_(), if it wasn't working you probably called it wrong. Can you send me over the line and the stacktrace?

@skateman I'm sorry, I did try to use _(singular_text, plural_text, number) but now I'm using n_() and it's works very well. Could you review again?

skateman · 2018-08-14T10:42:29Z

I got the tooltip, but the number is still there:

CharlleDaniel · 2018-08-22T12:03:29Z

@skateman and @mzazrivec could you see the comments ?

skateman · 2018-08-22T17:15:13Z

app/services/ems_physical_infra_dashboard_service.rb

-      :count     => count,
+  def format_tooltip(health_state, component_type, count)
+    singular_structure = "%{count} %{component_type} is in %{health_state} state."
+    plural_structure = "%{count} %{component_type} are in %{health_state} state."


@mzazrivec do we have something for this is/are thing?

This is fine. Though the strings need to be inside n_(). n_(var1, var2, count) won't work with the string extraction process.

@mzazrivec just a question, why n_(var1, var2, count) won't work? There must be only one string per n_( ) ?

The extraction logic cannot parse the string out if it's in a variable (or any expression).

@mzazrivec thanks. Is it fine now?

It's better than fine, it's gaprindashvili! 😆

Keep hammering that home.

@mzazrivec @skateman Could someone merge or approve this PR ?

miq-bot · 2018-08-27T12:43:40Z

Checked commit https://github.com/CharlleDaniel/manageiq-ui-classic/commit/f4071cab84200d9f0b8ef12b26504e72a758fc4b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

skateman

- Red crosses were added as part of ManageIQ#4453 that caused them to show up on all aggregate status cards on all provider dashboards, fixed the condition and removed setting of redundant notification hash for providers when it is not needed. - Show status icon under Alerts in aggregate card if Alerts are configured for a container provider, otherwise show "No data available" empty pf card. - On Containers Overview dashboard, Fixed to not show provider icon in Provider status aggregate card, it should only show count of providers with a link to list view to Container Providers show list. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1628384 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626427

- Red crosses were added as part of ManageIQ#4453 that caused them to show up on all aggregate status cards on all provider dashboards, fixed the condition and removed setting of redundant notification hash for providers when it is not needed. - Show status icon under Alerts in aggregate card if Alerts are configured for a container provider, otherwise show "No data available" empty pf card. - On Containers Overview dashboard, Fixed to not show provider icon in Provider status aggregate card, it should only show count of providers with a link to list view to Container Providers show list. - Added a title on Container Overview dashboard screen, since that's the only screen on that subtab there are no breadcrumbs on the screen. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1628384 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626427

- Red crosses were added as part of ManageIQ#4453 that caused them to show up on all aggregate status cards on all provider dashboards, fixed the condition and removed setting of redundant notification hash for providers when it is not needed. - Show status icon under Alerts in aggregate card if Alerts are configured for a container provider, otherwise show "No data available" empty pf card. - On Containers Overview dashboard, Fixed to not show provider icon in Provider status aggregate card, it should only show count of providers with a link to list view to Container Providers show list. - Added a title on Container Overview dashboard screen, since that's the only screen on that subtab there are no breadcrumbs on the screen. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626199 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626427

- Red crosses were added as part of ManageIQ#4453 that caused them to show up on all aggregate status cards on all provider dashboards, fixed the condition and removed setting of redundant notification hash for providers when it is not needed. - Show status icon under Alerts in aggregate card if Alerts are configured for a container provider, otherwise show "No data available" empty pf card. - On Containers Overview dashboard, Fixed to not show provider icon in Provider status aggregate card, it should only show count of providers with a link to list view to Container Providers show list. Adjusted existing spec test to expect iconImage to be nil. - Added a title on Container Overview dashboard screen, since that's the only screen on that subtab there are no breadcrumbs on the screen. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626199 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626427

- Red crosses were added as part of ManageIQ#4453 that caused them to show up on all aggregate status cards on all provider dashboards, fixed the condition and removed setting of redundant notification hash for providers when it is not needed. - Show status icon under Alerts in aggregate card if Alerts are configured for a container provider, otherwise show "No data available" empty pf card. - On Containers Overview dashboard, Fixed to not show provider icon in Provider status aggregate card, it should only show count of providers with a link to list view to Container Providers show list. - Added a title on Container Overview dashboard screen, since that's the only screen on that subtab there are no breadcrumbs on the screen. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626199 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626427

This comment has been minimized.

Sign in to view

CharlleDaniel changed the title ~~Add a tooltip message to health state of the cards~~ Add a tooltip to health state of the cards inside the physical infra dashboard Aug 10, 2018

CharlleDaniel changed the title ~~Add a tooltip to health state of the cards inside the physical infra dashboard~~ Add a tooltip to health state of the cards in the physical infra dashboard Aug 10, 2018

mzazrivec added enhancement ux/review gaprindashvili/no labels Aug 13, 2018

mzazrivec reviewed Aug 13, 2018

View reviewed changes

CharlleDaniel closed this Aug 13, 2018

CharlleDaniel reopened this Aug 13, 2018

miq-bot added ux/approved and removed ux/review labels Aug 14, 2018

mzazrivec reviewed Aug 14, 2018

View reviewed changes

skateman reviewed Aug 22, 2018

View reviewed changes

Add a tooltip message to health state of the cards

f4071ca

CharlleDaniel closed this Aug 27, 2018

CharlleDaniel reopened this Aug 27, 2018

CharlleDaniel closed this Aug 28, 2018

CharlleDaniel reopened this Aug 28, 2018

skateman approved these changes Aug 31, 2018

View reviewed changes

mzazrivec self-assigned this Aug 31, 2018

mzazrivec added this to the Sprint 94 Ending Sept 10, 2018 milestone Aug 31, 2018

mzazrivec merged commit d119f9b into ManageIQ:master Aug 31, 2018

h-kataria mentioned this pull request Sep 13, 2018

Fixed some inconsistencies on provider dashboards. #4659

Merged

Add a tooltip to health state of the cards in the physical infra dashboard #4453

Add a tooltip to health state of the cards in the physical infra dashboard #4453

Conversation

CharlleDaniel commented Aug 10, 2018 • edited

CharlleDaniel commented Aug 10, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

CharlleDaniel commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terezanovotna commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlleDaniel Aug 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman commented Aug 14, 2018

CharlleDaniel commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Aug 27, 2018

skateman left a comment

Choose a reason for hiding this comment

CharlleDaniel commented Aug 10, 2018 •

edited

CharlleDaniel Aug 14, 2018 •

edited