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] 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