Convert PNGs to font icons and SVGs in trees #79

epwinchell · 2017-01-05T15:41:05Z

This PR is a followup to: #8 , replacing images with font icons managed by TreeBuilder, where possible. https://www.pivotaltracker.com/story/show/129371557

epwinchell · 2017-01-05T15:41:32Z

@miq-bot add_label ui, technical debt, wip

miq-bot · 2017-01-05T15:42:22Z

@epwinchell Cannot apply the following label because they are not recognized: ui

skateman · 2017-01-05T15:51:53Z

app/presenters/tree_builder_chargeback_rates.rb

@@ -21,7 +21,7 @@ def x_get_tree_roots(count_only, options)
      else
        objects = []
        rate_types.sort.each do |rtype|
-          img = rtype.downcase == "compute" ? "100/hardware-processor.png" : "100/hardware-disk.png"
+          img = rtype.downcase == "compute" ? "product product-memory" : "fa fa-hdd-o"


This is converted to an icon, but it's still used as an image on line 28

@skateman fixed

skateman · 2017-01-05T15:54:41Z

app/presenters/tree_builder_report_reports.rb

@@ -30,7 +30,7 @@ def x_get_tree_roots(count_only, options)
      objects.push(
        :id    => i.to_s,
        :text  => r[0],
-        :image => (@grp_title == r[0] ? '100/blue_folder.png' : '100/folder.png'),
+        :icon  => (@grp_title == r[0] ? 'pficon pficon-folder-close-blue' : 'pficon pficon-folder-close'),


here you can remove the redundancy:

:icon => "pficon pficon-folder-close#{@grp_title == r[0] ? '-blue' : ''}",

skateman · 2017-01-05T15:54:57Z

app/presenters/tree_builder_report_reports.rb

@@ -47,7 +47,7 @@ def x_get_tree_custom_kids(object, count_only, _options)
        objects.push(
          :id    => "#{nodes.last.split('-').last}-#{i}",
          :text  => r[0],
-          :image => (@grp_title == @rpt_menu[nodes.last.to_i][0] ? '100/blue_folder.png' : '100/folder.png'),
+          :icon  => (@grp_title == @rpt_menu[nodes.last.to_i][0] ? 'pficon pficon-folder-close-blue' : 'pficon pficon-folder-close'),


skateman · 2017-01-05T15:56:04Z

app/presenters/tree_builder_vat.rb

@@ -12,7 +12,7 @@ def initialize(name, type, sandbox, build = true, root = nil, vat = nil)
  private

  def root_options
-    image = "100/vendor-#{@root.image_name}.png"
+    image = "svg/vendor-#{@root.image_name}.svg"


we will have to rethink these root_options because on some places it's an image, on some place it's an icon

Remove this, it should go into the same separate PR as add_root_node.

skateman · 2017-01-05T15:56:41Z

app/presenters/tree_node/compliance.rb

@@ -7,6 +7,6 @@ class Compliance < Node
      end
    end

-    set_attribute(:image) { "100/#{@object.compliant ? "check" : "x"}.png" }
+    set_attribute(:icon) { "pficon pficon-#{@object.compliant ? 'pficon pficon-ok' : 'pficon pficon-error-circle-o'}" }


don't duplicate the prefix:

set_attribute(:icon) { "pficon pficon-#{@object.compliant ? 'ok' : 'error-circle-o'}" }

skateman · 2017-01-05T15:57:05Z

app/presenters/tree_node/compliance_detail.rb

@@ -7,6 +7,6 @@ class ComplianceDetail < Node
      end
    end

-    set_attribute(:image) { "100/#{@object.miq_policy_result ? "check" : "x"}.png" }
+    set_attribute(:icon) { "pficon pficon-#{@object.miq_policy_result ? 'pficon pficon-ok' : 'pficon pficon-error-circle-o'}" }


don't duplicate the prefix:

set_attribute(:icon) { "pficon pficon-#{@object.miq_policy_result ? 'ok' : 'error-circle-o'}" }

skateman · 2017-01-05T15:57:51Z

app/presenters/tree_node/custom_button.rb

-    set_attribute(:image) do
-      @object.options && @object.options[:button_image] ? "100/custom-#{@object.options[:button_image]}.png" : '100/leaf.gif'
+    set_attribute(:icon) do
+      @object.options && @object.options[:button_image] ? "product product-custom-#{@object.options[:button_image]}" : 'fa fa-file-o'


are you 100% sure that product-custom-xxx always exists?

@skateman I'm not sure what you mean. We have fonticon equivalents of the 15 custom icons.

good enough for me

Yeah, I'd prefer to see another map here, but for now.. :)

skateman · 2017-01-05T15:58:49Z

app/presenters/tree_node/ems_folder.rb

        tooltip = _("Datacenter: %{datacenter_name}") % {:datacenter_name => @object.name}
      else
-        icon = %i(vandt vat).include?(@options[:type]) ? '100/blue_folder.png' : '100/folder.png'
+        icon = %i(vandt vat).include?(@options[:type]) ? 'pficon pficon-folder-close-blue' : 'pficon pficon-folder-close'


you can remove redundancy here:

icon = "pficon pficon-folder-close#{%i(vandt vat).include?(@options[:type]) ? '-blue' : ''}"

skateman · 2017-01-05T15:59:48Z

app/presenters/tree_node/miq_report_result.rb

@@ -3,15 +3,15 @@ class MiqReportResult < Node
    set_attribute(:image) do


set_attribute(:icon)

skateman · 2017-01-05T16:00:11Z

app/presenters/tree_node/orchestration_template.rb

      suffix = @object.class.name.underscore.split("_").last.downcase
      suffix = 'vapp' if suffix == 'template'
-      "100/orchestration_template_#{suffix}.png"
+      "product product-template"


What about the suffix here?

@skateman That was calling copies of the same "T" image - redundant.

miq-bot · 2017-01-06T10:42:52Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-01-06T22:03:37Z

This pull request is not mergeable. Please rebase and repush.

epwinchell · 2017-01-06T22:55:56Z

@miq-bot rm_label wip

epwinchell · 2017-01-07T00:28:27Z

@miq-bot assign @himdel

skateman

Except of the changes related to the root node it's all good. Nice job!

skateman · 2017-01-07T08:53:32Z

app/presenters/tree_builder.rb

@@ -176,7 +176,7 @@ def add_to_sandbox
  def add_root_node(nodes)
    root = nodes.first
    root[:title], root[:tooltip], icon, options = root_options
-    root[:image] = ActionController::Base.helpers.image_path(icon || "100/folder.png")
+    root[:icon] = icon || "pficon pficon-folder-close"


This should be in a separate PR, so remove this change please.

skateman · 2017-01-07T08:59:42Z

app/presenters/tree_builder_automate_simulation_results.rb

@@ -58,7 +58,7 @@ def get_element_title(el)

  def get_element_icon(el)
    icons = {
-      :MiqAeObject    => "100/q.png",
+      :MiqAeObject    => "svg/vendor-redhat.svg",
      :MiqAeAttribute => "100/attribute.png",
      :not_blank      => "100/#{el.name.underscore}.png",
      :other          => "100/#{el.name.underscore.sub(/^miq_ae_service_/, '')}.png",


Converting these would require something like:

def get_element_icon(el) icons = { :MiqAeObject => { :image => "svg/vendor-redhat.svg" }, :MiqAeAttribute => { :icon => "some fonticon" }, :not_blank => { :image => "100/#{el.name.underscore}.png" }, :other => {:image => "100/#{el.name.underscore.sub(/^miq_ae_service_/, '')}.png" }, } choose_correct_attr(el, icons) end

And then on lines 71-77:

object = {:id => "e_#{idx}", :text => title, :tip => title, :elements => el.each_element { |e| e }, :cfmeNoClick => true }.merge(get_element_icon(el))

@himdel what do you think?

skateman · 2017-01-07T09:02:55Z

app/presenters/tree_builder_vat.rb

@@ -12,7 +12,7 @@ def initialize(name, type, sandbox, build = true, root = nil, vat = nil)
  private

  def root_options
-    image = "100/vendor-#{@root.image_name}.png"
+    image = "svg/vendor-#{@root.image_name}.svg"


Remove this, it should go into the same separate PR as add_root_node.

skateman · 2017-01-07T09:07:04Z

spec/presenters/tree_builder_vat_spec.rb

@@ -21,7 +21,7 @@ def image_name

    it 'returns EmsCluster as root' do
      root = @vat_tree.send(:root_options)
-      image = "100/vendor-#{@vat_tree.instance_variable_get(:@root).image_name}.png"
+      image = "svg/vendor-#{@vat_tree.instance_variable_get(:@root).image_name}.svg"


Remove this change, same as add_root_node

miq-bot · 2017-01-09T11:27:48Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-01-09T16:17:58Z

This pull request is not mergeable. Please rebase and repush.

https://www.pivotaltracker.com/story/show/129371557

miq-bot · 2017-01-09T21:53:25Z

Checked commit https://github.com/epwinchell/manageiq-ui-classic/commit/b578497c5cfc51f41c81f7178face3a1573490f3 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
151 files checked, 61 offenses detected

app/presenters/tree_builder_catalog_items.rb

❕ - Line 54, Col 121 - Metrics/LineLength - Line is too long. [134/120]

app/presenters/tree_builder_chargeback_reports.rb

❗ - Line 31, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_iso_datastores.rb

❗ - Line 36, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_orchestration_templates.rb

❗ - Line 23, Col 8 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 28, Col 8 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 33, Col 8 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 38, Col 8 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 43, Col 8 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_policy.rb

❕ - Line 50, Col 121 - Metrics/LineLength - Line is too long. [141/120]
❕ - Line 51, Col 121 - Metrics/LineLength - Line is too long. [123/120]

app/presenters/tree_builder_provisioning_dialogs.rb

❗ - Line 18, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_pxe_customization_templates.rb

❗ - Line 29, Col 20 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❕ - Line 32, Col 121 - Metrics/LineLength - Line is too long. [130/120]

app/presenters/tree_builder_pxe_servers.rb

❗ - Line 38, Col 22 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 45, Col 22 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_report_export.rb

❗ - Line 26, Col 8 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 29, Col 8 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_report_reports.rb

❗ - Line 33, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 50, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❕ - Line 50, Col 121 - Metrics/LineLength - Line is too long. [129/120]

app/presenters/tree_builder_report_widgets.rb

❕ - Line 18, Col 121 - Metrics/LineLength - Line is too long. [124/120]

app/presenters/tree_builder_smartproxy_affinity.rb

❗ - Line 42, Col 8 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 56, Col 8 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_storage.rb

❕ - Line 20, Col 121 - Metrics/LineLength - Line is too long. [153/120]
❕ - Line 21, Col 121 - Metrics/LineLength - Line is too long. [152/120]

app/presenters/tree_builder_utilization.rb

❗ - Line 47, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_vms_filter.rb

❕ - Line 21, Col 121 - Metrics/LineLength - Line is too long. [153/120]
❕ - Line 22, Col 121 - Metrics/LineLength - Line is too long. [152/120]

app/presenters/tree_node/custom_button.rb

❕ - Line 4, Col 121 - Metrics/LineLength - Line is too long. [133/120]

app/presenters/tree_node/custom_button_set.rb

❕ - Line 12, Col 121 - Metrics/LineLength - Line is too long. [150/120]

app/presenters/tree_node/service_resource.rb

❗ - Line 4, Col 1 - Style/EmptyLinesAroundClassBody - Extra empty line detected at class body end.
❕ - Line 3, Col 121 - Metrics/LineLength - Line is too long. [131/120]

spec/presenters/tree_builder_report_widgets_spec.rb

❕ - Line 10, Col 121 - Metrics/LineLength - Line is too long. [167/120]
❕ - Line 11, Col 121 - Metrics/LineLength - Line is too long. [166/120]
❕ - Line 12, Col 121 - Metrics/LineLength - Line is too long. [169/120]
❕ - Line 13, Col 121 - Metrics/LineLength - Line is too long. [164/120]

spec/presenters/tree_builder_servers_by_role_spec.rb

❗ - Line 59, Col 17 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

spec/presenters/tree_builder_smartproxy_affinity_spec.rb

skateman · 2017-01-10T13:07:31Z

@himdel I think this is good to go!

himdel · 2017-01-10T14:26:45Z

app/presenters/tree_node/orchestration_template.rb

-      suffix = 'vapp' if suffix == 'template'
-      "100/orchestration_template_#{suffix}.png"
+    set_attribute(:icon) do
+      "product product-template"


wanted to say we're losing the type distinction here .. but looking at 100/orchestration_template_*.png they all look the same, so not saying anything :)

himdel · 2017-01-10T14:28:05Z

LGTM, merging :)

more core ext's on 4.1.0 update for update sake https://github.com/ManageIQ/more_core_extensions/compare/179bf40..e5b4501 - Added Ruby 2.7 support [[ManageIQ#79](ManageIQ/more_core_extensions#79)] - Added Process#pause, Process#resume, and Process#alive? [[ManageIQ#73](ManageIQ/more_core_extensions#73)] array added * `#compact_map` - Collect non-nil results from the block array added `#tabular_sort` - Sorts an Array of Hashes by specific columns hierarchy added `#descendant_get` - Returns the descendant with a given name the two breaking changes: - **BREAKING**: Moved Object#descendant_get to Class#descendant_get [[ManageIQ#75](ManageIQ/more_core_extensions#75)] - **BREAKING**: Removed deprecated Enumerable#stable_sort_by [[ManageIQ#76](ManageIQ/more_core_extensions#76)] a minor header output change was made that hasn't been released yet to make tableize more markdown compliant see ManageIQ/linux_admin#221

miq-bot added technical debt wip labels Jan 5, 2017

skateman suggested changes Jan 5, 2017

View reviewed changes

miq-bot added unmergeable and removed unmergeable labels Jan 6, 2017

epwinchell changed the title ~~[WIP] convert PNGs to font icons and SVGs in trees~~ Convert PNGs to font icons and SVGs in trees Jan 6, 2017

miq-bot removed the wip label Jan 6, 2017

miq-bot assigned himdel Jan 7, 2017

skateman suggested changes Jan 7, 2017

View reviewed changes

miq-bot added unmergeable and removed unmergeable labels Jan 9, 2017

convert pngs to font icons in trees

b578497

https://www.pivotaltracker.com/story/show/129371557

epwinchell closed this Jan 10, 2017

epwinchell reopened this Jan 10, 2017

himdel reviewed Jan 10, 2017

View reviewed changes

himdel merged commit c3cd859 into ManageIQ:master Jan 10, 2017

himdel added the euwe/no label Jan 10, 2017

himdel added this to the Sprint 52 Ending Jan 16, 2017 milestone Jan 10, 2017

epwinchell deleted the tree_node_icons branch October 25, 2017 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert PNGs to font icons and SVGs in trees #79

Convert PNGs to font icons and SVGs in trees #79

epwinchell commented Jan 5, 2017 •

edited

Loading

epwinchell commented Jan 5, 2017

miq-bot commented Jan 5, 2017

skateman Jan 5, 2017

epwinchell Jan 5, 2017

skateman Jan 5, 2017

skateman Jan 5, 2017

skateman Jan 5, 2017

skateman Jan 7, 2017

skateman Jan 5, 2017

skateman Jan 5, 2017

skateman Jan 5, 2017

epwinchell Jan 5, 2017

skateman Jan 5, 2017

himdel Jan 10, 2017

skateman Jan 5, 2017

skateman Jan 5, 2017

skateman Jan 5, 2017

epwinchell Jan 5, 2017

miq-bot commented Jan 6, 2017

miq-bot commented Jan 6, 2017

epwinchell commented Jan 6, 2017

epwinchell commented Jan 7, 2017

skateman left a comment

skateman Jan 7, 2017

skateman Jan 7, 2017

skateman Jan 7, 2017

skateman Jan 7, 2017

miq-bot commented Jan 9, 2017

miq-bot commented Jan 9, 2017

miq-bot commented Jan 9, 2017

skateman commented Jan 10, 2017

himdel Jan 10, 2017

himdel commented Jan 10, 2017

		@@ -3,15 +3,15 @@ class MiqReportResult < Node
		set_attribute(:image) do

Convert PNGs to font icons and SVGs in trees #79

Convert PNGs to font icons and SVGs in trees #79

Conversation

epwinchell commented Jan 5, 2017 • edited Loading

epwinchell commented Jan 5, 2017

miq-bot commented Jan 5, 2017

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

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 Jan 6, 2017

miq-bot commented Jan 6, 2017

epwinchell commented Jan 6, 2017

epwinchell commented Jan 7, 2017

skateman left a 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 Jan 9, 2017

miq-bot commented Jan 9, 2017

miq-bot commented Jan 9, 2017

skateman commented Jan 10, 2017

Choose a reason for hiding this comment

himdel commented Jan 10, 2017

epwinchell commented Jan 5, 2017 •

edited

Loading