Skip to content
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

TextualSummary: unify power states. #4085

Merged

Conversation

martinpovolny
Copy link
Member

@martinpovolny martinpovolny commented Jun 7, 2018

Unify the display of power state in textual summaries (detail screens).

Display a power state icon.

ping @terezanovotna, @Loicavenel

New

ts-phys-new

Old

ts-phys-old

@martinpovolny martinpovolny changed the title TextualSummary: unify power stated. [WIP] TextualSummary: unify power stated. Jun 7, 2018
@miq-bot miq-bot added the wip label Jun 7, 2018
@martinpovolny martinpovolny force-pushed the textual_summary_power_unification branch 2 times, most recently from 8b249eb to 94df913 Compare June 7, 2018 15:45
@martinpovolny martinpovolny changed the title [WIP] TextualSummary: unify power stated. TextualSummary: unify power stated. Jun 7, 2018
@martinpovolny martinpovolny force-pushed the textual_summary_power_unification branch from 94df913 to edaedce Compare June 7, 2018 15:59
@miq-bot miq-bot removed the wip label Jun 7, 2018
@martinpovolny martinpovolny force-pushed the textual_summary_power_unification branch from edaedce to 59296c6 Compare June 7, 2018 19:42
@terezanovotna
Copy link

Great job!!! that was quick, thanks so much!

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

@Loicavenel
Copy link

Nice... I like it

@martinpovolny martinpovolny force-pushed the textual_summary_power_unification branch from 59296c6 to b1f7b7e Compare June 8, 2018 10:01
@miq-bot
Copy link
Member

miq-bot commented Jun 8, 2018

Checked commit martinpovolny@b1f7b7e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 1 offense detected

app/helpers/vm_cloud_helper/textual_summary.rb

h = {:label => _("Power State"), :value => state}
h[:image] = "svg/currentstate-#{@record.template? ? 'template' : state}.svg"
h
VALID_POWER_STATE = %w(
Copy link
Contributor

@himdel himdel Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks suspiciously similar to QuadiconHelper#MACHINE_STATE_QUADRANT (except that one uses fonticons for those same states)

Should we try to unify those two as well?

Copy link
Member Author

@martinpovolny martinpovolny Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar, but not the same. Yes, we could and should unify that. But I don't have an authoritative set of values for this. That would have to come from the backend. Or do we know what are the valid values? @skateman ?

I see two options:

  • leave this as it is now. Possibly do some unification in the user, after we somehow figure what are the valid values.
  • use @skateman 's list from QuadionHelper ignoring the couple of extra values (powering*).

I don't have a strong opinion here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this for now, eventually we should be using fonticons instead of svgs here as well.

@himdel himdel self-assigned this Jun 11, 2018
@himdel himdel added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 11, 2018
@himdel himdel merged commit 6b60896 into ManageIQ:master Jun 11, 2018
@martinpovolny martinpovolny changed the title TextualSummary: unify power stated. TextualSummary: unify power states. Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants