From 1057f41d61024d383e0d98d1740d3416afaabf58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Mar 2024 19:25:20 +0100 Subject: [PATCH 1/6] Fix indentation in investments executions partial We accidentally introduced the wrong indentation in commit 8376efce3. --- .../budgets/executions/_investments.html.erb | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/app/views/budgets/executions/_investments.html.erb b/app/views/budgets/executions/_investments.html.erb index e0f3abfdd..50e45cab8 100644 --- a/app/views/budgets/executions/_investments.html.erb +++ b/app/views/budgets/executions/_investments.html.erb @@ -1,26 +1,26 @@ <% @investments_by_heading.each do |heading, investments| %> -

- <%= heading.name %> (<%= investments.count %>) -

-
- <% investments.each do |investment| %> -
-
- <%= render Budgets::Executions::ImageComponent.new(investment) %> -
-
-
- <%= link_to investment.title, - budget_investment_path(@budget, investment, anchor: "tab-milestones") %> -
- <%= investment.author.name %> -
-

- <%= investment.formatted_price %> -

+

+ <%= heading.name %> (<%= investments.count %>) +

+
+ <% investments.each do |investment| %> +
+
+ <%= render Budgets::Executions::ImageComponent.new(investment) %> +
+
+
+ <%= link_to investment.title, + budget_investment_path(@budget, investment, anchor: "tab-milestones") %> +
+ <%= investment.author.name %>
+

+ <%= investment.formatted_price %> +

- <% end %> -
+
+ <% end %> +
<% end %> From e3a2a42534b0550c01d05ea1b7e5e6dd68bd12e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Mar 2024 19:14:41 +0100 Subject: [PATCH 2/6] Move investments executions view to a component Note that we're changing the component so it uses `polymorphic_path`; that way we don't have to pass the `@budget` variable to the component. We could also use `budget_investment_path investment.budget, investment` instead. --- .../budgets/executions/investments_component.html.erb} | 4 ++-- app/components/budgets/executions/investments_component.rb | 7 +++++++ app/views/budgets/executions/show.html.erb | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) rename app/{views/budgets/executions/_investments.html.erb => components/budgets/executions/investments_component.html.erb} (84%) create mode 100644 app/components/budgets/executions/investments_component.rb diff --git a/app/views/budgets/executions/_investments.html.erb b/app/components/budgets/executions/investments_component.html.erb similarity index 84% rename from app/views/budgets/executions/_investments.html.erb rename to app/components/budgets/executions/investments_component.html.erb index 50e45cab8..b6a7dc937 100644 --- a/app/views/budgets/executions/_investments.html.erb +++ b/app/components/budgets/executions/investments_component.html.erb @@ -1,4 +1,4 @@ -<% @investments_by_heading.each do |heading, investments| %> +<% investments_by_heading.each do |heading, investments| %>

<%= heading.name %> (<%= investments.count %>)

@@ -11,7 +11,7 @@
<%= link_to investment.title, - budget_investment_path(@budget, investment, anchor: "tab-milestones") %> + polymorphic_path(investment, anchor: "tab-milestones") %>
<%= investment.author.name %>
diff --git a/app/components/budgets/executions/investments_component.rb b/app/components/budgets/executions/investments_component.rb new file mode 100644 index 000000000..d6f50bac7 --- /dev/null +++ b/app/components/budgets/executions/investments_component.rb @@ -0,0 +1,7 @@ +class Budgets::Executions::InvestmentsComponent < ApplicationComponent + attr_reader :investments_by_heading + + def initialize(investments_by_heading) + @investments_by_heading = investments_by_heading + end +end diff --git a/app/views/budgets/executions/show.html.erb b/app/views/budgets/executions/show.html.erb index 01c16fc44..4db70fb8f 100644 --- a/app/views/budgets/executions/show.html.erb +++ b/app/views/budgets/executions/show.html.erb @@ -45,7 +45,7 @@ <%= render Budgets::Executions::FiltersComponent.new(@budget, @statuses) %> <% if @investments_by_heading.any? %> - <%= render "budgets/executions/investments" %> + <%= render Budgets::Executions::InvestmentsComponent.new(@investments_by_heading) %> <% else %>
<%= t("budgets.executions.no_winner_investments") %> From 1e063e88c23bdec19c31bad55135366025c0fad5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Mar 2024 19:29:44 +0100 Subject: [PATCH 3/6] Extract component to render heading executions Note we're adding the `budgets-executions-heading` HTML class, which is consistent to what we do in other components. --- .../executions/heading_component.html.erb | 26 +++++++++++++++++++ .../budgets/executions/heading_component.rb | 8 ++++++ .../executions/investments_component.html.erb | 25 +----------------- 3 files changed, 35 insertions(+), 24 deletions(-) create mode 100644 app/components/budgets/executions/heading_component.html.erb create mode 100644 app/components/budgets/executions/heading_component.rb diff --git a/app/components/budgets/executions/heading_component.html.erb b/app/components/budgets/executions/heading_component.html.erb new file mode 100644 index 000000000..e4f67b94d --- /dev/null +++ b/app/components/budgets/executions/heading_component.html.erb @@ -0,0 +1,26 @@ +
+

+ <%= heading.name %> (<%= investments.count %>) +

+
+ <% investments.each do |investment| %> +
+
+ <%= render Budgets::Executions::ImageComponent.new(investment) %> +
+
+
+ <%= link_to investment.title, + polymorphic_path(investment, anchor: "tab-milestones") %> +
+ <%= investment.author.name %> +
+

+ <%= investment.formatted_price %> +

+
+
+
+ <% end %> +
+
diff --git a/app/components/budgets/executions/heading_component.rb b/app/components/budgets/executions/heading_component.rb new file mode 100644 index 000000000..4f38ac5a7 --- /dev/null +++ b/app/components/budgets/executions/heading_component.rb @@ -0,0 +1,8 @@ +class Budgets::Executions::HeadingComponent < ApplicationComponent + attr_reader :heading, :investments + + def initialize(heading, investments) + @heading = heading + @investments = investments + end +end diff --git a/app/components/budgets/executions/investments_component.html.erb b/app/components/budgets/executions/investments_component.html.erb index b6a7dc937..93ffbb281 100644 --- a/app/components/budgets/executions/investments_component.html.erb +++ b/app/components/budgets/executions/investments_component.html.erb @@ -1,26 +1,3 @@ <% investments_by_heading.each do |heading, investments| %> -

- <%= heading.name %> (<%= investments.count %>) -

-
- <% investments.each do |investment| %> -
-
- <%= render Budgets::Executions::ImageComponent.new(investment) %> -
-
-
- <%= link_to investment.title, - polymorphic_path(investment, anchor: "tab-milestones") %> -
- <%= investment.author.name %> -
-

- <%= investment.formatted_price %> -

-
-
-
- <% end %> -
+ <%= render Budgets::Executions::HeadingComponent.new(heading, investments) %> <% end %> From 764d22f57a37fee135b1c02d5252f00c42023cba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Mar 2024 20:09:25 +0100 Subject: [PATCH 4/6] Use flex instead of an equalizer in executions Just like we did in commits like f2e32b44b, a8537f7e1 and be9fc2265, we're replacing a buggy JavaScript solution with one using just CSS. Besides, we've had a failure in our test suite caused by an image not being displayed on the page, with the message: ``` Failures: 1) Executions Images renders last milestone's image if investment has multiple milestones with images associated Failure/Error: expect(page).to have_css("img[alt='Second image']") expected to find visible css "img[alt='Second image']" but there were no matches. Also found "", which matched the selector but not all filters. # ./spec/system/budgets/executions_spec.rb:135:in `block (3 levels) in ' ``` The text "matched the selector but not all filters" means that the element was present on the page but wasn't visible. One possible cause is that the equalizer was adjusting the height of the element containing the image before the image was loaded. Note that, after these changes, all investments on the same row will have the same height but, unlike with Foundation's equalizer, investments on different rows might have different heights. --- .../budgets/executions/heading.scss | 20 +++++++++++++ .../executions/heading_component.html.erb | 29 +++++++++---------- 2 files changed, 34 insertions(+), 15 deletions(-) create mode 100644 app/assets/stylesheets/budgets/executions/heading.scss diff --git a/app/assets/stylesheets/budgets/executions/heading.scss b/app/assets/stylesheets/budgets/executions/heading.scss new file mode 100644 index 000000000..605f0debc --- /dev/null +++ b/app/assets/stylesheets/budgets/executions/heading.scss @@ -0,0 +1,20 @@ +.budgets-executions-heading { + .budgets-executions-heading-investments { + $spacing: rem-calc(map-get($grid-column-gutter, medium)); + + @include flex-with-gap($spacing); + flex-wrap: wrap; + + > * { + margin-bottom: $line-height; + + @include breakpoint(medium) { + width: calc(100% / 2 - #{$spacing}); + } + + @include breakpoint(large) { + width: calc(100% / 3 - #{$spacing}); + } + } + } +} diff --git a/app/components/budgets/executions/heading_component.html.erb b/app/components/budgets/executions/heading_component.html.erb index e4f67b94d..812d3c0bc 100644 --- a/app/components/budgets/executions/heading_component.html.erb +++ b/app/components/budgets/executions/heading_component.html.erb @@ -2,23 +2,22 @@

<%= heading.name %> (<%= investments.count %>)

-
+ +
<% investments.each do |investment| %> -
-
- <%= render Budgets::Executions::ImageComponent.new(investment) %> -
-
-
- <%= link_to investment.title, - polymorphic_path(investment, anchor: "tab-milestones") %> -
- <%= investment.author.name %> -
-

- <%= investment.formatted_price %> -

+
+ <%= render Budgets::Executions::ImageComponent.new(investment) %> +
+
+
+ <%= link_to investment.title, + polymorphic_path(investment, anchor: "tab-milestones") %> +
+ <%= investment.author.name %>
+

+ <%= investment.formatted_price %> +

<% end %> From 75b03791b1c44dd53b7ee981f2c509ebe402a2dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Mar 2024 21:57:11 +0100 Subject: [PATCH 5/6] Extract component to render an execution Note that, in order to be consistent with the name of the component, we're renaming the `budget-execution` class to `budget-executions-investment`. --- .../budgets/executions/investment.scss | 54 ++++++++++++++++++ app/assets/stylesheets/participation.scss | 55 ------------------- .../executions/heading_component.html.erb | 16 +----- .../executions/investment_component.html.erb | 15 +++++ .../executions/investment_component.rb | 7 +++ spec/system/budgets/executions_spec.rb | 2 +- 6 files changed, 78 insertions(+), 71 deletions(-) create mode 100644 app/assets/stylesheets/budgets/executions/investment.scss create mode 100644 app/components/budgets/executions/investment_component.html.erb create mode 100644 app/components/budgets/executions/investment_component.rb diff --git a/app/assets/stylesheets/budgets/executions/investment.scss b/app/assets/stylesheets/budgets/executions/investment.scss new file mode 100644 index 000000000..bd8176af1 --- /dev/null +++ b/app/assets/stylesheets/budgets/executions/investment.scss @@ -0,0 +1,54 @@ +.budget-executions-investment { + @include card; + border: 1px solid $border; + overflow: hidden; + position: relative; + + a { + color: inherit; + display: block; + + img { + height: $line-height * 9; + max-width: none; + min-width: 100%; + transition-duration: 0.3s; + transition-property: transform; + } + + &:hover { + text-decoration: none; + + img { + transform: scale(1.05); + } + } + } + + h5 { + font-size: $base-font-size; + margin-bottom: 0; + } + + .budget-execution-info { + padding: calc(#{$line-height} / 2); + } + + .author { + color: $text-medium; + font-size: $small-font-size; + } + + .budget-execution-content { + min-height: $line-height * 3; + } + + .price { + color: $budget; + font-size: rem-calc(24); + } + + &:hover:not(:focus-within) { + box-shadow: 0 0 12px 0 rgba(0, 0, 0, 0.2); + } +} diff --git a/app/assets/stylesheets/participation.scss b/app/assets/stylesheets/participation.scss index 407b94077..2dd68776b 100644 --- a/app/assets/stylesheets/participation.scss +++ b/app/assets/stylesheets/participation.scss @@ -1290,61 +1290,6 @@ } } -.budget-execution { - @include card; - border: 1px solid $border; - overflow: hidden; - position: relative; - - a { - color: inherit; - display: block; - - img { - height: $line-height * 9; - max-width: none; - min-width: 100%; - transition-duration: 0.3s; - transition-property: transform; - } - - &:hover { - text-decoration: none; - - img { - transform: scale(1.05); - } - } - } - - h5 { - font-size: $base-font-size; - margin-bottom: 0; - } - - .budget-execution-info { - padding: calc(#{$line-height} / 2); - } - - .author { - color: $text-medium; - font-size: $small-font-size; - } - - .budget-execution-content { - min-height: $line-height * 3; - } - - .price { - color: $budget; - font-size: rem-calc(24); - } - - &:hover:not(:focus-within) { - box-shadow: 0 0 12px 0 rgba(0, 0, 0, 0.2); - } -} - // 07. Proposals successful // ------------------------- diff --git a/app/components/budgets/executions/heading_component.html.erb b/app/components/budgets/executions/heading_component.html.erb index 812d3c0bc..6a1599527 100644 --- a/app/components/budgets/executions/heading_component.html.erb +++ b/app/components/budgets/executions/heading_component.html.erb @@ -5,21 +5,7 @@
<% investments.each do |investment| %> -
- <%= render Budgets::Executions::ImageComponent.new(investment) %> -
-
-
- <%= link_to investment.title, - polymorphic_path(investment, anchor: "tab-milestones") %> -
- <%= investment.author.name %> -
-

- <%= investment.formatted_price %> -

-
-
+ <%= render Budgets::Executions::InvestmentComponent.new(investment) %> <% end %>
diff --git a/app/components/budgets/executions/investment_component.html.erb b/app/components/budgets/executions/investment_component.html.erb new file mode 100644 index 000000000..c990b2193 --- /dev/null +++ b/app/components/budgets/executions/investment_component.html.erb @@ -0,0 +1,15 @@ +
+ <%= render Budgets::Executions::ImageComponent.new(investment) %> +
+
+
+ <%= link_to investment.title, + polymorphic_path(investment, anchor: "tab-milestones") %> +
+ <%= investment.author.name %> +
+

+ <%= investment.formatted_price %> +

+
+
diff --git a/app/components/budgets/executions/investment_component.rb b/app/components/budgets/executions/investment_component.rb new file mode 100644 index 000000000..4ba229d5d --- /dev/null +++ b/app/components/budgets/executions/investment_component.rb @@ -0,0 +1,7 @@ +class Budgets::Executions::InvestmentComponent < ApplicationComponent + attr_reader :investment + + def initialize(investment) + @investment = investment + end +end diff --git a/spec/system/budgets/executions_spec.rb b/spec/system/budgets/executions_spec.rb index 5b721e257..eb0044e78 100644 --- a/spec/system/budgets/executions_spec.rb +++ b/spec/system/budgets/executions_spec.rb @@ -294,7 +294,7 @@ describe "Executions" do visit budget_executions_path(budget) - expect(page).to have_css(".budget-execution", count: 3) + expect(page).to have_css(".budget-executions-investment", count: 3) expect(a_heading.name).to appear_before(m_heading.name) expect(m_heading.name).to appear_before(z_heading.name) end From 10cdfadb9b9897a33b847d3b7d99ce03cd82a1eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 29 Oct 2024 01:46:58 +0100 Subject: [PATCH 6/6] Move executions images system tests to the component We forgot to do so in commit f0ab7bcfc. We're still leaving one system spec to test that these images are rendered when browsing the site. We're also changing the existing component tests to check the `alt` attribute, just like the remaining system test does. --- .../executions/image_component_spec.rb | 43 ++++++++++++++++-- spec/system/budgets/executions_spec.rb | 44 +------------------ 2 files changed, 42 insertions(+), 45 deletions(-) diff --git a/spec/components/budgets/executions/image_component_spec.rb b/spec/components/budgets/executions/image_component_spec.rb index a40d8d1ce..4cead4790 100644 --- a/spec/components/budgets/executions/image_component_spec.rb +++ b/spec/components/budgets/executions/image_component_spec.rb @@ -1,13 +1,50 @@ require "rails_helper" describe Budgets::Executions::ImageComponent do + let(:component) { Budgets::Executions::ImageComponent.new(investment) } + + context "investment with image" do + let(:investment) { create(:budget_investment, :with_image) } + + it "shows a milestone image if available" do + create(:milestone, :with_image, image_title: "Building in progress", milestoneable: investment) + + render_inline component + + expect(page).to have_css "img[alt='Building in progress']" + expect(page).not_to have_css "img[alt='#{investment.image.title}']" + end + + it "shows the investment image if no milestone image is available" do + create(:milestone, :with_image, image_title: "Not related", milestoneable: create(:budget_investment)) + + render_inline component + + expect(page).to have_css "img[alt='#{investment.image.title}']" + expect(page).not_to have_css "img[alt='Not related']" + end + + it "shows the last milestone's image if the investment has multiple milestones with images associated" do + create(:milestone, milestoneable: investment) + create(:milestone, :with_image, image_title: "First image", milestoneable: investment) + create(:milestone, :with_image, image_title: "Second image", milestoneable: investment) + create(:milestone, milestoneable: investment) + + render_inline component + + expect(page).to have_css "img[alt='Second image']" + expect(page).not_to have_css "img[alt='First image']" + expect(page).not_to have_css "img[alt='#{investment.image.title}']" + end + end + context "investment and milestone without image" do - let(:component) { Budgets::Executions::ImageComponent.new(Budget::Investment.new) } + let(:investment) { Budget::Investment.new(title: "New and empty") } it "shows the default image" do render_inline component - expect(page).to have_css "img[src*='budget_execution_no_image']" + expect(page).to have_css "img[src*='budget_execution_no_image'][alt='New and empty']" end it "shows a custom default image when available" do @@ -18,7 +55,7 @@ describe Budgets::Executions::ImageComponent do render_inline component - expect(page).to have_css "img[src$='logo_header-260x80.png']" + expect(page).to have_css "img[src$='logo_header-260x80.png'][alt='New and empty']" expect(page).not_to have_css "img[src*='budget_execution_no_image']" end end diff --git a/spec/system/budgets/executions_spec.rb b/spec/system/budgets/executions_spec.rb index eb0044e78..88d21ec02 100644 --- a/spec/system/budgets/executions_spec.rb +++ b/spec/system/budgets/executions_spec.rb @@ -83,31 +83,6 @@ describe "Executions" do end context "Images" do - scenario "renders milestone image if available" do - milestone1 = create(:milestone, :with_image, milestoneable: investment1) - - visit budget_path(budget) - - click_link "See results" - click_link "Milestones" - - expect(page).to have_content(investment1.title) - expect(page).to have_css("img[alt='#{milestone1.image.title}']") - end - - scenario "renders investment image if no milestone image is available" do - create(:milestone, milestoneable: investment2) - create(:image, imageable: investment2) - - visit budget_path(budget) - - click_link "See results" - click_link "Milestones" - - expect(page).to have_content(investment2.title) - expect(page).to have_css("img[alt='#{investment2.image.title}']") - end - scenario "renders default image if no milestone nor investment images are available" do create(:milestone, milestoneable: investment4) @@ -116,23 +91,8 @@ describe "Executions" do click_link "See results" click_link "Milestones" - expect(page).to have_content(investment4.title) - expect(page).to have_css("img[alt='#{investment4.title}']") - end - - scenario "renders last milestone's image if investment has multiple milestones with images associated" do - create(:milestone, milestoneable: investment1) - create(:milestone, :with_image, image_title: "First image", milestoneable: investment1) - create(:milestone, :with_image, image_title: "Second image", milestoneable: investment1) - create(:milestone, milestoneable: investment1) - - visit budget_path(budget) - - click_link "See results" - click_link "Milestones" - - expect(page).to have_content(investment1.title) - expect(page).to have_css("img[alt='Second image']") + expect(page).to have_content investment4.title + expect(page).to have_css "img[alt='#{investment4.title}']" end end