-
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
Physical Server quadicon #1173
Physical Server quadicon #1173
Conversation
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.
Could you please provide a screenshot of the new quadicon in the PR's description?
app/helpers/quadicon_helper.rb
Outdated
end | ||
end | ||
end | ||
safe_join(output.collect(&:html_safe)) |
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.
Is this html_safe
really necessary when using safe_join
?
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.
No, it wasn't. I just removed it.
app/helpers/quadicon_helper.rb
Outdated
@@ -641,6 +699,15 @@ def render_single_quad_quadicon(item, options) | |||
output.collect(&:html_safe).join('').html_safe | |||
end | |||
|
|||
def img_for_health_state(item) | |||
case item.health_state | |||
when "Valid" then "100/healthstate-normal.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.
@MaysaMacedo this is changed in this PR: #1166 . The new path to this image is: svg/healthstate-normal.svg
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.
OK. Just changed it.
Just one more small thing, can you squash the two commits and make the commit message the same as the PR's title? |
1a2c25b
to
576d53b
Compare
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.
I'm impressed you found your way through this code, it's a bit of a mess 😄 It mostly looks great, but it would be helpful if the methods that are specific to a physical server could be moved to a decorator. I'd like to refactor this code to be more OO and I'd like the quadicon builders to ask the objects where their icons are. Along those lines it would be super helpful if you could add some specs so that when I refactor I don't break your code 😁
app/helpers/quadicon_helper.rb
Outdated
@@ -151,6 +151,10 @@ def render_quadicon(item, options = {}) | |||
end | |||
end | |||
|
|||
def img_for_physical_vendor(item) |
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.
Could this be moved to a decorator? (here's an example) We're trying to clean up this code and that would help :)
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.
I've just moved this to a decorator.
@@ -641,6 +699,15 @@ def render_single_quad_quadicon(item, options) | |||
output.collect(&:html_safe).join('').html_safe | |||
end | |||
|
|||
def img_for_health_state(item) |
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.
It would be cool if this could be moved into the decorator as well.
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.
I'm impressive with all ManageIQ architecture, then I think that refactoring which you proposed can be think after this PR be merged, because this feature it's taking more time than supposed to.
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.
I'm fine with keeping this here (for now), but let's ask @himdel'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.
Agreed, better have it together with the 4 other methods here, than in an unexpected place by itself. 👍
if options[:typ] == :listnav | ||
# Listnav, no href needed | ||
output << content_tag(:div, :class => 'flobj') do | ||
tag(:img, :src => ActionController::Base.helpers.image_path("layout/reflection.png"), :border => 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.
I know tag(:img)
exists elsewhere in this code, but I don't think it's necessary. Should be able to use quadicon_reflection_img
here. Plus, I think doing this will clear up the CodeClimate complaint.
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.
ok, I just changed 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.
Thanks @MaysaMacedo !
@dclarizio I'm 👍 on this when green.
"svg/vendor-#{label_for_vendor.downcase}.svg" | ||
end | ||
|
||
def img_for_health_state |
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.
health_state_fileicon
sounds better, but let's ask @epwinchell &| @himdel
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.
Just following the MIQ pattern.(e. g.: img_for_compliance
, img_for_vendor
, img_for_host_vendor
and so on...)
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.
AFAIK decorators don't have this pattern yet, so I'd like to ask the guys from above to check this out to not have a bad pattern that gets copied to other places 😉
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.
AFAIK decorators there isn't any method wich has a img for a specific property yet as you realized, so to follow(not copy) a previous pattern sounds good for me. But we'll wait for other reviewers. ;)
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.
Yeah, this is weird. In fact I'd just keep it where it was, since we have no standard way of doing this in decorators yet.
I mean, decorators are really useful if you want to get an icon for something you know is a model, but not sure which one.
Any use of the decorators for a method that is not available in any other decorator is ..a bit suspect..
(I mean, what's the point of doing @record.decorate.img_for_health_state
if you know the type of the record and can't ever call it on any other kind.)
But.. this is a tech debt either way, so.. not saying you have to move it out, just that there is no point in putting it there :).
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.
But: if you're creating a new decorator, please make sure it has one or both of the fonticon
and fileicon
methods, which return the right thing for that entity.
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.
I agree with also having fonticon
and fileicon
methods. But the point of moving these other methods closer to the model is that they are more related to PhysicalServer. It's not super clear whose responsibility an image path is, but it's more likely the model's than the QuadiconHelper module. I also agree that having to know the type @record
isn't great, but most of this code is type-checks on @record
anyway.
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.
Just added fonticon
and fileicon
methods, and also fixed the typo.
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 images are fine. We'll replace them later with font icons.
"svg/vendor-#{label_for_vendor.downcase}.svg" | ||
end | ||
|
||
def img_for_health_state |
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.
AFAIK decorators there isn't any method wich has a img for a specific property yet as you realized, so to follow(not copy) a previous pattern sounds good for me. But we'll wait for other reviewers. ;)
app/helpers/quadicon_helper.rb
Outdated
end | ||
|
||
def quadicon_named_for_base_model?(item) | ||
%w(VmOrTempalte PhysicalServer).include?(item.class.base_model.name) |
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.
typo.. tempalte != template
Except for the typo, and missing |
@himdel in the case that doen't exists a useful to physical server fonticon/fileicon yet, we just need return nil? I need this information to fix this issues, can you help me? |
@walteraa Sure.. I would not recommend returning |
@walteraa if the icons are the same for each item regardless of their state you should to define |
18cfa2a
to
39a8a8a
Compare
"svg/vendor-#{vendor.downcase}.svg" | ||
end | ||
|
||
def fonticon |
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.
As this method contains a static string, it is better to declare it as self.fonticon
.
Thanks @MaysaMacedo, I just noticed one more thing..
So.. any time you call (In Second, sorry for the confusion, but I'll ask you to move the other method ( (Also, what @skateman said about |
58d8586
to
25b7a34
Compare
fc0d428
to
25b7a34
Compare
Added Physical Server quadicon
d3c0cbb
to
e56ae4d
Compare
Closing to run travis again. |
@dclarizio Is this PR ready for a merge? Or there any additional comments? |
@rodneyhbrown7 @MaysaMacedo I'm fine with this PR, but I'd wait for an ACK from @himdel. He's off this week and will be back on monday ... |
@himdel feel free to merge if you think this is ready |
@himdel Can we merge this? |
Sorry, doesn't work...
|
app/helpers/quadicon_helper.rb
Outdated
output << flobj_img_simple(img_for_health_state(item), "d72") | ||
output << flobj_img_simple('100/shield.png', "g72") unless item.get_policies.empty? | ||
else | ||
output << flobj_img_simple(size) |
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.
this size
is never defined ..and does not really make sense because flobj_img_simple
expects an image as a first arg
EDIT: Seems like other similar branches use either flobj_img_simple
without args or flobj_img_simple("layout/base-single.png")
(which is identical to the no args version).
So.. just remove that (size)
?
app/helpers/quadicon_helper.rb
Outdated
output << flobj_img_simple('100/shield.png', "g72") unless item.get_policies.empty? | ||
else | ||
output << flobj_img_simple(size) | ||
output << flobj_img_simple(width * 1.8, item.ext_management_system.decorate.fileicon, "e72") |
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.
And the first arg here is dead too.. output << flobj_img_simple(item.ext_management_system.decorate.fileicon, "e72")
should do the trick.
Another thing missing here is the My Settings change to be able to actually enable those quadicons. @MaysaMacedo you're adding the default values for The right place to expose this is My Settings > Visual > Grid/Tile Icons ( EDIT: this seems to do the trick... diff --git a/app/controllers/configuration_controller.rb b/app/controllers/configuration_controller.rb
index 7e4e64592f..0f5e77d504 100644
--- a/app/controllers/configuration_controller.rb
+++ b/app/controllers/configuration_controller.rb
@@ -580,6 +580,7 @@ class ConfigurationController < ApplicationController
@edit[:new][:quadicons][:ems_cloud] = params[:quadicons_ems_cloud] == "true" if params[:quadicons_ems_cloud]
@edit[:new][:quadicons][:host] = params[:quadicons_host] == "true" if params[:quadicons_host]
@edit[:new][:quadicons][:vm] = params[:quadicons_vm] == "true" if params[:quadicons_vm]
+ @edit[:new][:quadicons][:physical_server] = params[:quadicons_physical_server] == "true" if params[:quadicons_physical_server]
@edit[:new][:quadicons][:miq_template] = params[:quadicons_miq_template] == "true" if params[:quadicons_miq_template]
if ::Settings.product.proto # Hide behind proto setting - Sprint 34
@edit[:new][:quadicons][:service] = params[:quadicons_service] == "true" if params[:quadicons_service]
diff --git a/app/helpers/quadicon_helper.rb b/app/helpers/quadicon_helper.rb
index 909a14b7a0..0281977af9 100644
--- a/app/helpers/quadicon_helper.rb
+++ b/app/helpers/quadicon_helper.rb
@@ -230,8 +230,8 @@ module QuadiconHelper
output << flobj_img_simple(img_for_health_state(item), "d72")
output << flobj_img_simple('100/shield.png', "g72") unless item.get_policies.empty?
else
- output << flobj_img_simple(size)
- output << flobj_img_simple(width * 1.8, item.ext_management_system.decorate.fileicon, "e72")
+ output << flobj_img_simple
+ output << flobj_img_simple(item.ext_management_system.decorate.fileicon, "e72")
end
if options[:typ] == :listnav
diff --git a/app/views/configuration/_ui_1.html.haml b/app/views/configuration/_ui_1.html.haml
index 4756cf2113..28dd2339e9 100644
--- a/app/views/configuration/_ui_1.html.haml
+++ b/app/views/configuration/_ui_1.html.haml
@@ -18,6 +18,7 @@
[role_allows?(:feature => "host_show_list"), _("Host"), "host"],
[role_allows?(:feature => "storage_show_list"), ui_lookup(:table => "storages"), "storage"],
[true, _("VM"), "vm"],
+ [true, _("Physical Server"), "physical_server"],
[true, _("Template"), "miq_template"]].each do |icons_checkbox|
- if icons_checkbox[0]
.form-group |
app/helpers/quadicon_helper.rb
Outdated
output << flobj_img_simple("layout/base.png") | ||
|
||
output << flobj_p_simple("a72", (item.host ? 1 : 0)) | ||
output << flobj_img_simple("svg/currentstate-#{h(item.power_state.downcase)}.svg", "b72") |
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.
may want to use item.power_state.try(:downcase)
- hard to tell without real data, but this crashes when a server does not have a power_state
which is technically possible
app/helpers/quadicon_helper.rb
Outdated
case item.health_state | ||
when "Valid" then "svg/healthstate-normal.svg" | ||
when "Critical" then "svg/healthstate-critical.svg" | ||
when "None" then "svg/healthstate-unknown.svg" |
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.
Again hard to tell without real data, but when health_state
is nil, you get an ugly base-single in the fourth quadrant, may want to make "None"
the default case.
e56ae4d
to
5a1e210
Compare
Checked commits MaysaMacedo/manageiq-ui-classic@1acec52~...5a1e210 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@himdel I've made the changes you asked for. Thanks for the heads up. |
Thanks, verified in the UI, both branches now work 👍 .. merged :) |
This PR contains:
depends on: