From 9beb1608c42c80f0e829f8cc2f5d26dd30505ddd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 29 Mar 2024 20:27:22 +0100 Subject: [PATCH 1/9] Remove alt attribute in avatar images These images are always displayed next to a username, meaning people using screen readers were hearing the same username twice in a row. Even though we're about to replace the initialjs gem, we're making this change in case so we've got one more test and we can check everything keeps working after replacing the gem. --- app/components/shared/avatar_component.rb | 2 +- spec/components/shared/avatar_component_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 spec/components/shared/avatar_component_spec.rb diff --git a/app/components/shared/avatar_component.rb b/app/components/shared/avatar_component.rb index a54618ec4..bd9fa7813 100644 --- a/app/components/shared/avatar_component.rb +++ b/app/components/shared/avatar_component.rb @@ -10,7 +10,7 @@ class Shared::AvatarComponent < ApplicationComponent private def default_options - { background_color: colors[seed % colors.size] } + { background_color: colors[seed % colors.size], alt: "" } end def options diff --git a/spec/components/shared/avatar_component_spec.rb b/spec/components/shared/avatar_component_spec.rb new file mode 100644 index 000000000..3fbb83a85 --- /dev/null +++ b/spec/components/shared/avatar_component_spec.rb @@ -0,0 +1,10 @@ +require "rails_helper" + +describe Shared::AvatarComponent do + it "does not contain redundant text already present around it" do + render_inline Shared::AvatarComponent.new(double(id: 1, name: "Johnny")) + + expect(page).to have_css "img", count: 1 + expect(page).to have_css "img[alt='']" + end +end From d3b1b21d3d90deb43acddd971642232f77753d7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 8 Apr 2024 03:04:04 +0200 Subject: [PATCH 2/9] Extract matcher to check for avatars We're going to change the code to render avatars, and having a matcher will make it easier. --- spec/support/common_actions/comments.rb | 4 ---- spec/support/matchers/have_avatar.rb | 9 +++++++++ spec/system/account_spec.rb | 4 ++-- spec/system/debates_spec.rb | 6 +++--- spec/system/proposals_spec.rb | 6 +++--- spec/system/users_spec.rb | 4 ++-- 6 files changed, 19 insertions(+), 14 deletions(-) create mode 100644 spec/support/matchers/have_avatar.rb diff --git a/spec/support/common_actions/comments.rb b/spec/support/common_actions/comments.rb index 370e16fd9..1f600470b 100644 --- a/spec/support/common_actions/comments.rb +++ b/spec/support/common_actions/comments.rb @@ -18,8 +18,4 @@ module Comments end expect(page).to have_content "It will be done next week." end - - def avatar(name) - "img.initialjs-avatar[data-name='#{name}']" - end end diff --git a/spec/support/matchers/have_avatar.rb b/spec/support/matchers/have_avatar.rb new file mode 100644 index 000000000..d68757ceb --- /dev/null +++ b/spec/support/matchers/have_avatar.rb @@ -0,0 +1,9 @@ +RSpec::Matchers.define :have_avatar do |name, **options| + match do + has_css?("img.initialjs-avatar[data-name='#{name}'][src^='data:image/svg']", **options) + end + + failure_message do + "expected to find avatar with name #{name} but there were no matches." + end +end diff --git a/spec/system/account_spec.rb b/spec/system/account_spec.rb index 6d23e854b..d98d2f7ff 100644 --- a/spec/system/account_spec.rb +++ b/spec/system/account_spec.rb @@ -15,7 +15,7 @@ describe "Account" do expect(page).to have_current_path(account_path, ignore_query: true) expect(page).to have_css "input[value='Manuela Colau']" - expect(page).to have_css avatar("Manuela Colau"), count: 1 + expect(page).to have_avatar "Manuela Colau", count: 1 end scenario "Show organization" do @@ -26,7 +26,7 @@ describe "Account" do expect(page).to have_css "input[value='Manuela Corp']" expect(page).not_to have_css "input[value='Manuela Colau']" - expect(page).to have_css avatar("Manuela Corp"), count: 1 + expect(page).to have_avatar "Manuela Corp", count: 1 end scenario "Edit" do diff --git a/spec/system/debates_spec.rb b/spec/system/debates_spec.rb index fa70d949f..ba83f2a1f 100644 --- a/spec/system/debates_spec.rb +++ b/spec/system/debates_spec.rb @@ -72,15 +72,15 @@ describe "Debates" do end scenario "Show" do - debate = create(:debate) + debate = create(:debate, author: create(:user, username: "Charles Dickens")) visit debate_path(debate) expect(page).to have_content debate.title expect(page).to have_content "Debate description" - expect(page).to have_content debate.author.name + expect(page).to have_content "Charles Dickens" expect(page).to have_content I18n.l(debate.created_at.to_date) - expect(page).to have_css avatar(debate.author.name) + expect(page).to have_avatar "Charles Dickens" expect(page.html).to include "#{debate.title}" end diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb index 6df959d81..13d1d8fd3 100644 --- a/spec/system/proposals_spec.rb +++ b/spec/system/proposals_spec.rb @@ -114,16 +114,16 @@ describe "Proposals" do end scenario "Show" do - proposal = create(:proposal) + proposal = create(:proposal, author: create(:user, username: "Mark Twain")) visit proposal_path(proposal) expect(page).to have_content proposal.title expect(page).to have_content proposal.code expect(page).to have_content "Proposal description" - expect(page).to have_content proposal.author.name + expect(page).to have_content "Mark Twain" expect(page).to have_content I18n.l(proposal.created_at.to_date) - expect(page).to have_css avatar(proposal.author.name) + expect(page).to have_avatar "Mark Twain" expect(page.html).to include "#{proposal.title}" expect(page).not_to have_css ".js-flag-actions" expect(page).not_to have_css ".js-follow" diff --git a/spec/system/users_spec.rb b/spec/system/users_spec.rb index bae98741f..2f1ca0bf4 100644 --- a/spec/system/users_spec.rb +++ b/spec/system/users_spec.rb @@ -488,14 +488,14 @@ describe "Users" do describe "Initials" do scenario "display SVG avatars when loaded into the DOM" do - login_as(create(:user)) + login_as(create(:user, username: "Commentator")) visit debate_path(create(:debate)) fill_in "Leave your comment", with: "I'm awesome" click_button "Publish comment" within ".comment", text: "I'm awesome" do - expect(page).to have_css "img.initialjs-avatar[src^='data:image/svg']" + expect(page).to have_avatar "Commentator" end end end From 35659d4419e036285e821d4188642273e733fadc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 29 Mar 2024 20:01:45 +0100 Subject: [PATCH 3/9] Replace initialjs-rails with custom avatar code The initialjs-rails gem hasn't been maintained for years, and it currently requires `railties < 7.0`, meaning we can't upgrade to Rails 7 while we depend on it. Since the code in the gem is simple, and we were already rewriting its most complex part (generating a background color), we can implement the same code, only we're using Ruby instead of JavaScript. This way, the avatars will be shown on browsers without JavaScript as well. Since we're adding a component test that checks SVG images are displayed even without JavaScript, we no longer need the test that checked images were displayed after AJAX requests. Now the tests show the user experience better; people don't care about the internal name used to select the initial (which is what we were checking); they care about the initial actually displayed. Note initialjs generated an tag using a `src="data:image/svg+xml;` attribute. We're generating an tag instead, because it's easier. For this reason, we need to change the code slightly, giving the tag the `img` role and using `aria-label` so its contents won't be read aloud by screen readers. We could give it a `presentation` role instead and forget about `aria-label`, but then screen readers would read the text anyway (or, at least, some of them would). --- Gemfile | 1 - Gemfile.lock | 3 -- app/assets/javascripts/application.js | 3 -- app/assets/javascripts/users.js | 15 ------ app/assets/stylesheets/application.scss | 1 + app/assets/stylesheets/avatar.scss | 4 ++ app/assets/stylesheets/layout.scss | 3 +- .../shared/avatar_component.html.erb | 2 +- app/components/shared/avatar_component.rb | 52 ++++++++++++++++++- .../shared/avatar_component_spec.rb | 20 +++++-- spec/support/matchers/have_avatar.rb | 6 +-- spec/system/account_spec.rb | 4 +- spec/system/debates_spec.rb | 2 +- spec/system/proposals_spec.rb | 2 +- spec/system/users_spec.rb | 14 ----- 15 files changed, 81 insertions(+), 51 deletions(-) delete mode 100644 app/assets/javascripts/users.js create mode 100644 app/assets/stylesheets/avatar.scss diff --git a/Gemfile b/Gemfile index a57e2c11b..fdd7d9c47 100644 --- a/Gemfile +++ b/Gemfile @@ -30,7 +30,6 @@ gem "graphiql-rails", "~> 1.8.0" gem "graphql", "~> 1.13.22" gem "groupdate", "~> 6.4.0" gem "image_processing", "~> 1.12.2" -gem "initialjs-rails", "~> 0.2.0.9" gem "invisible_captcha", "~> 2.3.0" gem "kaminari", "~> 1.2.2" gem "mini_magick", "~> 4.12.0" diff --git a/Gemfile.lock b/Gemfile.lock index f950e905d..537e4b816 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -280,8 +280,6 @@ GEM image_processing (1.12.2) mini_magick (>= 4.9.5, < 5) ruby-vips (>= 2.0.17, < 3) - initialjs-rails (0.2.0.9) - railties (>= 3.1, < 7.0) invisible_captcha (2.3.0) rails (>= 5.2) json (2.7.1) @@ -719,7 +717,6 @@ DEPENDENCIES groupdate (~> 6.4.0) i18n-tasks (~> 0.9.37) image_processing (~> 1.12.2) - initialjs-rails (~> 0.2.0.9) invisible_captcha (~> 2.3.0) kaminari (~> 1.2.2) knapsack_pro (~> 7.0.1) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index c6d884ea0..449ab33dc 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -58,7 +58,6 @@ //= require ckeditor/loader //= require_directory ./ckeditor //= require social-share-button -//= require initial //= require ahoy //= require app //= require check_all_none @@ -75,7 +74,6 @@ //= require annotator //= require jquery.amsify.suggestags //= require tags -//= require users //= require participation_not_allowed //= require advanced_search //= require registration_form @@ -136,7 +134,6 @@ var initialize_modules = function() { App.Answers.initialize(); App.Questions.initialize(); App.Comments.initialize(); - App.Users.initialize(); App.ParticipationNotAllowed.initialize(); App.Tags.initialize(); App.FoundationExtras.initialize(); diff --git a/app/assets/javascripts/users.js b/app/assets/javascripts/users.js deleted file mode 100644 index 8608f4d30..000000000 --- a/app/assets/javascripts/users.js +++ /dev/null @@ -1,15 +0,0 @@ -(function() { - "use strict"; - App.Users = { - initialize: function() { - var observer; - $(".initialjs-avatar").initial(); - observer = new MutationObserver(function(mutations) { - $.each(mutations, function(index, mutation) { - $(mutation.addedNodes).find(".initialjs-avatar").initial(); - }); - }); - observer.observe(document.body, { childList: true, subtree: true }); - } - }; -}).call(this); diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 623a4d185..abb94a533 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -25,6 +25,7 @@ @import "advanced_search"; @import "annotator_overrides"; @import "autocomplete_overrides"; +@import "avatar"; @import "banner"; @import "comments_count"; @import "datepicker_overrides"; diff --git a/app/assets/stylesheets/avatar.scss b/app/assets/stylesheets/avatar.scss new file mode 100644 index 000000000..e9f07cb16 --- /dev/null +++ b/app/assets/stylesheets/avatar.scss @@ -0,0 +1,4 @@ +.initialjs-avatar { + font-family: HelveticaNeue-Light, "Helvetica Neue Light", "Helvetica Neue", + Helvetica, Arial, "Lucida Grande", sans-serif; +} diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index 2469052d8..195cc2db8 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -1558,7 +1558,8 @@ table { .comment-body, .notification-body { - img { + img, + svg { margin-right: calc(#{$line-height} / 2); } diff --git a/app/components/shared/avatar_component.html.erb b/app/components/shared/avatar_component.html.erb index d84a07cea..59c18bb35 100644 --- a/app/components/shared/avatar_component.html.erb +++ b/app/components/shared/avatar_component.html.erb @@ -1 +1 @@ -<%= avatar_image(record, options) %> +<%= avatar_image %> diff --git a/app/components/shared/avatar_component.rb b/app/components/shared/avatar_component.rb index bd9fa7813..40ce21f9b 100644 --- a/app/components/shared/avatar_component.rb +++ b/app/components/shared/avatar_component.rb @@ -1,6 +1,5 @@ class Shared::AvatarComponent < ApplicationComponent attr_reader :record, :given_options - use_helpers :avatar_image def initialize(record, **given_options) @record = record @@ -10,7 +9,7 @@ class Shared::AvatarComponent < ApplicationComponent private def default_options - { background_color: colors[seed % colors.size], alt: "" } + { background_color: colors[seed % colors.size], size: 100, color: "white" } end def options @@ -27,4 +26,53 @@ class Shared::AvatarComponent < ApplicationComponent def seed record.id end + + def size + options[:size] + end + + def font_size + (size * 0.6).round + end + + def background_color + options[:background_color] + end + + def color + options[:color] + end + + def svg_options + { + xmlns: "http://www.w3.org/2000/svg", + width: size, + height: size, + role: "img", + "aria-label": "", + style: "background-color: #{background_color}", + class: "initialjs-avatar #{options[:class]}".strip + } + end + + def text_options + { + x: "50%", + y: "50%", + dy: "0.35em", + "text-anchor": "middle", + fill: color, + style: "font-size: #{font_size}px" + } + end + + def initial + record.name.first.upcase + end + + def avatar_image + tag.svg(**svg_options) do + tag.text(initial, **text_options) + end + end end diff --git a/spec/components/shared/avatar_component_spec.rb b/spec/components/shared/avatar_component_spec.rb index 3fbb83a85..cc1d7b1e1 100644 --- a/spec/components/shared/avatar_component_spec.rb +++ b/spec/components/shared/avatar_component_spec.rb @@ -1,10 +1,22 @@ require "rails_helper" describe Shared::AvatarComponent do - it "does not contain redundant text already present around it" do - render_inline Shared::AvatarComponent.new(double(id: 1, name: "Johnny")) + let(:user) { double(id: 1, name: "Johnny") } + let(:component) { Shared::AvatarComponent.new(user) } - expect(page).to have_css "img", count: 1 - expect(page).to have_css "img[alt='']" + it "does not contain redundant text already present around it" do + render_inline component + + expect(page).to have_css "svg", count: 1 + expect(page).to have_css "svg[role='img'][aria-label='']" + end + + it "shows the initial letter of the name" do + render_inline component + + page.find("svg") do |avatar| + expect(avatar).to have_text "J" + expect(avatar).not_to have_text "Johnny" + end end end diff --git a/spec/support/matchers/have_avatar.rb b/spec/support/matchers/have_avatar.rb index d68757ceb..b41ed1372 100644 --- a/spec/support/matchers/have_avatar.rb +++ b/spec/support/matchers/have_avatar.rb @@ -1,9 +1,9 @@ -RSpec::Matchers.define :have_avatar do |name, **options| +RSpec::Matchers.define :have_avatar do |text, **options| match do - has_css?("img.initialjs-avatar[data-name='#{name}'][src^='data:image/svg']", **options) + has_css?("svg.initialjs-avatar", **{ exact_text: text }.merge(options)) end failure_message do - "expected to find avatar with name #{name} but there were no matches." + "expected to find avatar with text #{text} but there were no matches." end end diff --git a/spec/system/account_spec.rb b/spec/system/account_spec.rb index d98d2f7ff..04c2d7e17 100644 --- a/spec/system/account_spec.rb +++ b/spec/system/account_spec.rb @@ -15,7 +15,7 @@ describe "Account" do expect(page).to have_current_path(account_path, ignore_query: true) expect(page).to have_css "input[value='Manuela Colau']" - expect(page).to have_avatar "Manuela Colau", count: 1 + expect(page).to have_avatar "M", count: 1 end scenario "Show organization" do @@ -26,7 +26,7 @@ describe "Account" do expect(page).to have_css "input[value='Manuela Corp']" expect(page).not_to have_css "input[value='Manuela Colau']" - expect(page).to have_avatar "Manuela Corp", count: 1 + expect(page).to have_avatar "M", count: 1 end scenario "Edit" do diff --git a/spec/system/debates_spec.rb b/spec/system/debates_spec.rb index ba83f2a1f..68d5de88c 100644 --- a/spec/system/debates_spec.rb +++ b/spec/system/debates_spec.rb @@ -80,7 +80,7 @@ describe "Debates" do expect(page).to have_content "Debate description" expect(page).to have_content "Charles Dickens" expect(page).to have_content I18n.l(debate.created_at.to_date) - expect(page).to have_avatar "Charles Dickens" + expect(page).to have_avatar "C" expect(page.html).to include "#{debate.title}" end diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb index 13d1d8fd3..47db7e6ba 100644 --- a/spec/system/proposals_spec.rb +++ b/spec/system/proposals_spec.rb @@ -123,7 +123,7 @@ describe "Proposals" do expect(page).to have_content "Proposal description" expect(page).to have_content "Mark Twain" expect(page).to have_content I18n.l(proposal.created_at.to_date) - expect(page).to have_avatar "Mark Twain" + expect(page).to have_avatar "M" expect(page.html).to include "#{proposal.title}" expect(page).not_to have_css ".js-flag-actions" expect(page).not_to have_css ".js-follow" diff --git a/spec/system/users_spec.rb b/spec/system/users_spec.rb index 2f1ca0bf4..6e35977b9 100644 --- a/spec/system/users_spec.rb +++ b/spec/system/users_spec.rb @@ -485,18 +485,4 @@ describe "Users" do expect(page).not_to have_content("Sport") end end - - describe "Initials" do - scenario "display SVG avatars when loaded into the DOM" do - login_as(create(:user, username: "Commentator")) - visit debate_path(create(:debate)) - - fill_in "Leave your comment", with: "I'm awesome" - click_button "Publish comment" - - within ".comment", text: "I'm awesome" do - expect(page).to have_avatar "Commentator" - end - end - end end From 2c9c5d9cd4caa479f3d1a22ef3f69b622b92a250 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Apr 2024 16:28:55 +0200 Subject: [PATCH 4/9] Extract component to render avatars in comments This way it'll be easier to add tests for it and refactor it. --- .../comments/avatar_component.html.erb | 13 +++++ app/components/comments/avatar_component.rb | 7 +++ app/views/comments/_comment.html.erb | 14 +---- .../comments/avatar_component_spec.rb | 56 +++++++++++++++++++ spec/support/matchers/have_avatar.rb | 4 +- 5 files changed, 79 insertions(+), 15 deletions(-) create mode 100644 app/components/comments/avatar_component.html.erb create mode 100644 app/components/comments/avatar_component.rb create mode 100644 spec/components/comments/avatar_component_spec.rb diff --git a/app/components/comments/avatar_component.html.erb b/app/components/comments/avatar_component.html.erb new file mode 100644 index 000000000..ea3a2c379 --- /dev/null +++ b/app/components/comments/avatar_component.html.erb @@ -0,0 +1,13 @@ +<% if comment.as_administrator? %> + <%= image_tag("avatar_admin.png", size: 32, class: "admin-avatar float-left") %> +<% elsif comment.as_moderator? %> + <%= image_tag("avatar_moderator.png", size: 32, class: "moderator-avatar float-left") %> +<% else %> +<% if comment.user.hidden? || comment.user.erased? %> + + <% elsif comment.user.organization? %> + <%= image_tag("avatar_collective.png", size: 32, class: "avatar float-left") %> + <% else %> + <%= render Shared::AvatarComponent.new(comment.user, size: 32, class: "float-left") %> + <% end %> +<% end %> diff --git a/app/components/comments/avatar_component.rb b/app/components/comments/avatar_component.rb new file mode 100644 index 000000000..d39fd9f2b --- /dev/null +++ b/app/components/comments/avatar_component.rb @@ -0,0 +1,7 @@ +class Comments::AvatarComponent < ApplicationComponent + attr_reader :comment + + def initialize(comment) + @comment = comment + end +end diff --git a/app/views/comments/_comment.html.erb b/app/views/comments/_comment.html.erb index 7b7a67c6e..aafcb2b9d 100644 --- a/app/views/comments/_comment.html.erb +++ b/app/views/comments/_comment.html.erb @@ -9,19 +9,7 @@ <% end %> <% else %> - <% if comment.as_administrator? %> - <%= image_tag("avatar_admin.png", size: 32, class: "admin-avatar float-left") %> - <% elsif comment.as_moderator? %> - <%= image_tag("avatar_moderator.png", size: 32, class: "moderator-avatar float-left") %> - <% else %> - <% if comment.user.hidden? || comment.user.erased? %> - - <% elsif comment.user.organization? %> - <%= image_tag("avatar_collective.png", size: 32, class: "avatar float-left") %> - <% else %> - <%= render Shared::AvatarComponent.new(comment.user, size: 32, class: "float-left") %> - <% end %> - <% end %> + <%= render Comments::AvatarComponent.new(comment) %>
diff --git a/spec/components/comments/avatar_component_spec.rb b/spec/components/comments/avatar_component_spec.rb new file mode 100644 index 000000000..173a24c44 --- /dev/null +++ b/spec/components/comments/avatar_component_spec.rb @@ -0,0 +1,56 @@ +require "rails_helper" + +describe Comments::AvatarComponent do + it "displays a regular avatar for regular comments" do + comment = create(:comment, user: create(:user, username: "Oscar Wilde")) + + render_inline Comments::AvatarComponent.new(comment) + + expect(page).to have_avatar "O" + expect(page).not_to have_css "img" + end + + it "displays the admin avatar for admin comments" do + admin = create(:administrator) + comment = create(:comment, user: admin.user, administrator_id: admin.id) + + render_inline Comments::AvatarComponent.new(comment) + + expect(page).to have_css "img.admin-avatar" + end + + it "displays the moderator avatar for moderator comments" do + moderator = create(:moderator) + comment = create(:comment, user: moderator.user, moderator_id: moderator.id) + + render_inline Comments::AvatarComponent.new(comment) + + expect(page).to have_css "img.moderator-avatar" + end + + it "displays the organization avatar for organization comments" do + comment = create(:comment, user: create(:organization).user) + + render_inline Comments::AvatarComponent.new(comment) + + expect(page).to have_css "img.avatar" + end + + it "displays an empty icon for comments by hidden users" do + comment = create(:comment, user: create(:user, :hidden)) + + render_inline Comments::AvatarComponent.new(comment) + + expect(page).to have_css ".user-deleted" + expect(page).not_to have_css "img" + end + + it "displays an empty icon for comments by erased users" do + comment = create(:comment, user: create(:user, erased_at: Time.current)) + + render_inline Comments::AvatarComponent.new(comment) + + expect(page).to have_css ".user-deleted" + expect(page).not_to have_css "img" + end +end diff --git a/spec/support/matchers/have_avatar.rb b/spec/support/matchers/have_avatar.rb index b41ed1372..f45895e84 100644 --- a/spec/support/matchers/have_avatar.rb +++ b/spec/support/matchers/have_avatar.rb @@ -1,6 +1,6 @@ RSpec::Matchers.define :have_avatar do |text, **options| - match do - has_css?("svg.initialjs-avatar", **{ exact_text: text }.merge(options)) + match do |page| + page.has_css?("svg.initialjs-avatar", **{ exact_text: text }.merge(options)) end failure_message do From c29ad265c6ee4c94936d917d6771c578e709a010 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 8 Apr 2024 02:44:02 +0200 Subject: [PATCH 5/9] Add missing alt attribute to special avatars The `alt` attribute is mandatory in image tags. In this case, we're leaving it empty because we also display text showing whether comments are made by administrators, moderators or organizations. --- app/components/comments/avatar_component.html.erb | 6 +++--- spec/components/comments/avatar_component_spec.rb | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/components/comments/avatar_component.html.erb b/app/components/comments/avatar_component.html.erb index ea3a2c379..9dd1c08f8 100644 --- a/app/components/comments/avatar_component.html.erb +++ b/app/components/comments/avatar_component.html.erb @@ -1,12 +1,12 @@ <% if comment.as_administrator? %> - <%= image_tag("avatar_admin.png", size: 32, class: "admin-avatar float-left") %> + <%= image_tag("avatar_admin.png", size: 32, class: "admin-avatar float-left", alt: "") %> <% elsif comment.as_moderator? %> - <%= image_tag("avatar_moderator.png", size: 32, class: "moderator-avatar float-left") %> + <%= image_tag("avatar_moderator.png", size: 32, class: "moderator-avatar float-left", alt: "") %> <% else %> <% if comment.user.hidden? || comment.user.erased? %> <% elsif comment.user.organization? %> - <%= image_tag("avatar_collective.png", size: 32, class: "avatar float-left") %> + <%= image_tag("avatar_collective.png", size: 32, class: "avatar float-left", alt: "") %> <% else %> <%= render Shared::AvatarComponent.new(comment.user, size: 32, class: "float-left") %> <% end %> diff --git a/spec/components/comments/avatar_component_spec.rb b/spec/components/comments/avatar_component_spec.rb index 173a24c44..f9e0a2857 100644 --- a/spec/components/comments/avatar_component_spec.rb +++ b/spec/components/comments/avatar_component_spec.rb @@ -10,30 +10,30 @@ describe Comments::AvatarComponent do expect(page).not_to have_css "img" end - it "displays the admin avatar for admin comments" do + it "displays the admin avatar with an empty alt attribute for admin comments" do admin = create(:administrator) comment = create(:comment, user: admin.user, administrator_id: admin.id) render_inline Comments::AvatarComponent.new(comment) - expect(page).to have_css "img.admin-avatar" + expect(page).to have_css "img.admin-avatar[alt='']" end - it "displays the moderator avatar for moderator comments" do + it "displays the moderator avatar with an empty alt attribute for moderator comments" do moderator = create(:moderator) comment = create(:comment, user: moderator.user, moderator_id: moderator.id) render_inline Comments::AvatarComponent.new(comment) - expect(page).to have_css "img.moderator-avatar" + expect(page).to have_css "img.moderator-avatar[alt='']" end - it "displays the organization avatar for organization comments" do + it "displays the organization avatar with an empty alt attribute for organization comments" do comment = create(:comment, user: create(:organization).user) render_inline Comments::AvatarComponent.new(comment) - expect(page).to have_css "img.avatar" + expect(page).to have_css "img.avatar[alt='']" end it "displays an empty icon for comments by hidden users" do From 7beb28bf89a48d1304c0f24a9b0aa487aa90effa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Apr 2024 16:56:24 +0200 Subject: [PATCH 6/9] Simplify the conditions to render comment avatars It was hard to understand that the nested `if` could actually be `elsif`, and the indentation was a bit broken. --- app/components/comments/avatar_component.html.erb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/app/components/comments/avatar_component.html.erb b/app/components/comments/avatar_component.html.erb index 9dd1c08f8..0c95fa943 100644 --- a/app/components/comments/avatar_component.html.erb +++ b/app/components/comments/avatar_component.html.erb @@ -2,12 +2,10 @@ <%= image_tag("avatar_admin.png", size: 32, class: "admin-avatar float-left", alt: "") %> <% elsif comment.as_moderator? %> <%= image_tag("avatar_moderator.png", size: 32, class: "moderator-avatar float-left", alt: "") %> +<% elsif comment.user.hidden? || comment.user.erased? %> + +<% elsif comment.user.organization? %> + <%= image_tag("avatar_collective.png", size: 32, class: "avatar float-left", alt: "") %> <% else %> -<% if comment.user.hidden? || comment.user.erased? %> - - <% elsif comment.user.organization? %> - <%= image_tag("avatar_collective.png", size: 32, class: "avatar float-left", alt: "") %> - <% else %> - <%= render Shared::AvatarComponent.new(comment.user, size: 32, class: "float-left") %> - <% end %> + <%= render Shared::AvatarComponent.new(comment.user, size: 32, class: "float-left") %> <% end %> From 67b1518858f75b6f0d217496f5845b34ae3ef5e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Apr 2024 17:01:12 +0200 Subject: [PATCH 7/9] Make comment avatars compatible with RTL languages --- app/assets/stylesheets/application.scss | 1 + app/assets/stylesheets/comments/avatar.scss | 6 +++++ app/assets/stylesheets/layout.scss | 2 +- .../comments/avatar_component.html.erb | 24 ++++++++++--------- 4 files changed, 21 insertions(+), 12 deletions(-) create mode 100644 app/assets/stylesheets/comments/avatar.scss diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index abb94a533..766bfa909 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -49,6 +49,7 @@ @import "account/**/*"; @import "admin/**/*"; @import "budgets/**/*"; +@import "comments/**/*"; @import "debates/**/*"; @import "documents/**/*"; @import "layout/**/*"; diff --git a/app/assets/stylesheets/comments/avatar.scss b/app/assets/stylesheets/comments/avatar.scss new file mode 100644 index 000000000..b47002a91 --- /dev/null +++ b/app/assets/stylesheets/comments/avatar.scss @@ -0,0 +1,6 @@ +.comment-avatar { + img, + svg { + float: $global-left; + } +} diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index 195cc2db8..febe1eb35 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -1560,7 +1560,7 @@ table { img, svg { - margin-right: calc(#{$line-height} / 2); + margin-#{$global-right}: calc(#{$line-height} / 2); } .reply { diff --git a/app/components/comments/avatar_component.html.erb b/app/components/comments/avatar_component.html.erb index 0c95fa943..432e3a33e 100644 --- a/app/components/comments/avatar_component.html.erb +++ b/app/components/comments/avatar_component.html.erb @@ -1,11 +1,13 @@ -<% if comment.as_administrator? %> - <%= image_tag("avatar_admin.png", size: 32, class: "admin-avatar float-left", alt: "") %> -<% elsif comment.as_moderator? %> - <%= image_tag("avatar_moderator.png", size: 32, class: "moderator-avatar float-left", alt: "") %> -<% elsif comment.user.hidden? || comment.user.erased? %> - -<% elsif comment.user.organization? %> - <%= image_tag("avatar_collective.png", size: 32, class: "avatar float-left", alt: "") %> -<% else %> - <%= render Shared::AvatarComponent.new(comment.user, size: 32, class: "float-left") %> -<% end %> + + <% if comment.as_administrator? %> + <%= image_tag("avatar_admin.png", size: 32, class: "admin-avatar", alt: "") %> + <% elsif comment.as_moderator? %> + <%= image_tag("avatar_moderator.png", size: 32, class: "moderator-avatar", alt: "") %> + <% elsif comment.user.hidden? || comment.user.erased? %> + + <% elsif comment.user.organization? %> + <%= image_tag("avatar_collective.png", size: 32, class: "avatar", alt: "") %> + <% else %> + <%= render Shared::AvatarComponent.new(comment.user, size: 32) %> + <% end %> + From c655eddddec63a60b29e68165959fb7055f85204 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Apr 2024 17:15:41 +0200 Subject: [PATCH 8/9] Extract method to render special avatars in comments --- app/components/comments/avatar_component.html.erb | 6 +++--- app/components/comments/avatar_component.rb | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/app/components/comments/avatar_component.html.erb b/app/components/comments/avatar_component.html.erb index 432e3a33e..dc59e90b4 100644 --- a/app/components/comments/avatar_component.html.erb +++ b/app/components/comments/avatar_component.html.erb @@ -1,12 +1,12 @@ <% if comment.as_administrator? %> - <%= image_tag("avatar_admin.png", size: 32, class: "admin-avatar", alt: "") %> + <%= special_avatar("avatar_admin.png", class: "admin-avatar") %> <% elsif comment.as_moderator? %> - <%= image_tag("avatar_moderator.png", size: 32, class: "moderator-avatar", alt: "") %> + <%= special_avatar("avatar_moderator.png", class: "moderator-avatar") %> <% elsif comment.user.hidden? || comment.user.erased? %> <% elsif comment.user.organization? %> - <%= image_tag("avatar_collective.png", size: 32, class: "avatar", alt: "") %> + <%= special_avatar("avatar_collective.png", class: "avatar") %> <% else %> <%= render Shared::AvatarComponent.new(comment.user, size: 32) %> <% end %> diff --git a/app/components/comments/avatar_component.rb b/app/components/comments/avatar_component.rb index d39fd9f2b..2fd691bf7 100644 --- a/app/components/comments/avatar_component.rb +++ b/app/components/comments/avatar_component.rb @@ -4,4 +4,10 @@ class Comments::AvatarComponent < ApplicationComponent def initialize(comment) @comment = comment end + + private + + def special_avatar(image_name, options = {}) + image_tag(image_name, { size: 32, alt: "" }.merge(options)) + end end From ccaa873e2a1f065aeb5a08bc3db0b3eb885123c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Apr 2024 17:18:02 +0200 Subject: [PATCH 9/9] Use Ruby instead of ERB to render comment avatars Reading conditions in Ruby is much easier than reading them in ERB and, since the block only had only HTML tag (the tag for deleted users) but was using Ruby in all other four cases, we're moving it to a Ruby file. --- app/components/comments/avatar_component.html.erb | 12 +----------- app/components/comments/avatar_component.rb | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/app/components/comments/avatar_component.html.erb b/app/components/comments/avatar_component.html.erb index dc59e90b4..a9cc6fde0 100644 --- a/app/components/comments/avatar_component.html.erb +++ b/app/components/comments/avatar_component.html.erb @@ -1,13 +1,3 @@ - <% if comment.as_administrator? %> - <%= special_avatar("avatar_admin.png", class: "admin-avatar") %> - <% elsif comment.as_moderator? %> - <%= special_avatar("avatar_moderator.png", class: "moderator-avatar") %> - <% elsif comment.user.hidden? || comment.user.erased? %> - - <% elsif comment.user.organization? %> - <%= special_avatar("avatar_collective.png", class: "avatar") %> - <% else %> - <%= render Shared::AvatarComponent.new(comment.user, size: 32) %> - <% end %> + <%= avatar %> diff --git a/app/components/comments/avatar_component.rb b/app/components/comments/avatar_component.rb index 2fd691bf7..9d1504658 100644 --- a/app/components/comments/avatar_component.rb +++ b/app/components/comments/avatar_component.rb @@ -7,6 +7,20 @@ class Comments::AvatarComponent < ApplicationComponent private + def avatar + if comment.as_administrator? + special_avatar("avatar_admin.png", class: "admin-avatar") + elsif comment.as_moderator? + special_avatar("avatar_moderator.png", class: "moderator-avatar") + elsif comment.user.hidden? || comment.user.erased? + tag.span(class: "icon-deleted user-deleted") + elsif comment.user.organization? + special_avatar("avatar_collective.png", class: "avatar") + else + render Shared::AvatarComponent.new(comment.user, size: 32) + end + end + def special_avatar(image_name, options = {}) image_tag(image_name, { size: 32, alt: "" }.merge(options)) end