From 9754d14182395793a8b280a904bae4303bbcd2bb Mon Sep 17 00:00:00 2001 From: PJ Date: Thu, 27 Jul 2023 19:48:07 +0100 Subject: [PATCH] Fix intermittently broken reaction images on home feed (#19779) * move reaction icons to base layout * fix reactions count specs?? * also fix article specs --- .../i18n_support.js => __support__/i18n.js} | 0 app/javascript/__support__/reaction_images.js | 22 +++++++++ .../articles/__tests__/Article.test.jsx | 5 ++ .../__snapshots__/Article.test.jsx.snap | 46 +++++++++++++++++++ .../articles/components/ReactionsCount.jsx | 10 ++-- .../__tests__/ReactionsCount.test.jsx | 43 ++++++++++++----- .../components/__tests__/FollowUsers.test.jsx | 2 +- app/views/articles/index.html.erb | 2 - app/views/collections/show.html.erb | 2 - app/views/layouts/application.html.erb | 1 + app/views/organizations/_main_feed.html.erb | 2 - .../stories/articles_search/index.html.erb | 2 - .../tagged_articles/_main_feed.html.erb | 2 - app/views/users/_main_feed.html.erb | 2 - config/reactions.yml | 3 ++ testSetup.js | 15 ++++++ 16 files changed, 132 insertions(+), 27 deletions(-) rename app/javascript/{utilities/i18n_support.js => __support__/i18n.js} (100%) create mode 100644 app/javascript/__support__/reaction_images.js diff --git a/app/javascript/utilities/i18n_support.js b/app/javascript/__support__/i18n.js similarity index 100% rename from app/javascript/utilities/i18n_support.js rename to app/javascript/__support__/i18n.js diff --git a/app/javascript/__support__/reaction_images.js b/app/javascript/__support__/reaction_images.js new file mode 100644 index 000000000..4c6e9e616 --- /dev/null +++ b/app/javascript/__support__/reaction_images.js @@ -0,0 +1,22 @@ +const fs = require('fs'); +const yaml = require('js-yaml'); + +const locale = './config/reactions.yml'; +const data = fs.readFileSync(locale, 'utf8'); +const yamlData = yaml.load(data); + +const publicReactionIcons = Object.keys(yamlData) + .filter( + (slug) => + yamlData[slug].privileged !== true && yamlData[slug].published !== false, + ) + .map((slug) => { + const { name, icon, position } = yamlData[slug]; + + return ``; + }) + .join(''); + +export function reactionImagesSupport() { + document.body.innerHTML += ``; +} diff --git a/app/javascript/articles/__tests__/Article.test.jsx b/app/javascript/articles/__tests__/Article.test.jsx index eab824d1e..bfe18e12c 100644 --- a/app/javascript/articles/__tests__/Article.test.jsx +++ b/app/javascript/articles/__tests__/Article.test.jsx @@ -4,6 +4,7 @@ import { render } from '@testing-library/preact'; import { axe } from 'jest-axe'; import '@testing-library/jest-dom'; import { Article } from '..'; +import { reactionImagesSupport } from '../../__support__/reaction_images'; import { locale } from '../../utilities/locale'; import { article, @@ -27,6 +28,10 @@ const commonProps = { }; describe('
component', () => { + beforeAll(() => { + reactionImagesSupport(); + }); + it('should have no a11y violations for a standard article', async () => { const { container } = render(
component should render a rich feed 1`] = ` Object { "asFragment": [Function], "baseElement": +
{ reversable.reverse(); const icons = reversable.map((category) => { - const path = reactionIcons?.querySelector( + const path = reactionIcons.querySelector( `img[data-slug=${category.slug}]`, - )?.src; + ).src; const alt = category.name; return ( @@ -33,7 +33,11 @@ export const ReactionsCount = ({ article }) => { }); return ( - + {icons} ); diff --git a/app/javascript/articles/components/__tests__/ReactionsCount.test.jsx b/app/javascript/articles/components/__tests__/ReactionsCount.test.jsx index 5c9c2b112..4c406705d 100644 --- a/app/javascript/articles/components/__tests__/ReactionsCount.test.jsx +++ b/app/javascript/articles/components/__tests__/ReactionsCount.test.jsx @@ -1,6 +1,8 @@ import { h } from 'preact'; -import { render } from '@testing-library/preact'; +import { render, within } from '@testing-library/preact'; import '@testing-library/jest-dom'; +import { i18nSupport } from '../../../__support__/i18n'; +import { reactionImagesSupport } from '../../../__support__/reaction_images'; import { ReactionsCount } from '..'; import { articleWithReactions, @@ -9,12 +11,17 @@ import { } from '../../__tests__/utilities/articleUtilities.js'; describe(' component', () => { + beforeAll(() => { + i18nSupport(); + reactionImagesSupport(); + }); + it('should not display reactions data when there are no reactions', async () => { const { queryByText } = render( , ); - expect(queryByText('0 reactions')).toBeNull(); + expect(queryByText(/0 reactions/i)).not.toExist(); }); it('should display reaction count when there are exactly one reaction', async () => { @@ -22,26 +29,40 @@ describe(' component', () => { , ); - expect(queryByText('1 reaction')).toBeNull(); + expect(queryByText(/1 reaction/i)).toExist(); }); it('should display reactions data when there are reactions', async () => { - const { findByText } = render( + const { queryByText } = render( , ); - expect(findByText('232 reactions')).toBeDefined(); + expect(queryByText(/232 reactions/i)).toExist(); }); it('should display multiple reactions when there are reactions', async () => { - const { findByText } = render( + const { getByTestId } = render( , ); - expect( - findByText( - 'Like', - ), - ).toBeDefined(); + const container = getByTestId('multiple-reactions-icons-container'); + const { queryByAltText } = within(container); + + // `articleWithReactions` has all reactions except exploding head + // also, we are not using `toHaveAttribute` directly because Jest inserts a + // base URL and we are only interested in the path + expect(queryByAltText('Like').getAttribute('src')).toContain( + '/assets/sparkle-heart.svg', + ); + expect(queryByAltText('Unicorn').getAttribute('src')).toContain( + '/assets/multi-unicorn.svg', + ); + expect(queryByAltText('Fire').getAttribute('src')).toContain( + '/assets/fire.svg', + ); + expect(queryByAltText('Raised Hands').getAttribute('src')).toContain( + '/assets/raised-hands.svg', + ); + expect(queryByAltText('Exploding Head')).not.toExist(); }); }); diff --git a/app/javascript/onboarding/components/__tests__/FollowUsers.test.jsx b/app/javascript/onboarding/components/__tests__/FollowUsers.test.jsx index fd7ceef3b..03db10bba 100644 --- a/app/javascript/onboarding/components/__tests__/FollowUsers.test.jsx +++ b/app/javascript/onboarding/components/__tests__/FollowUsers.test.jsx @@ -4,7 +4,7 @@ import fetch from 'jest-fetch-mock'; import '@testing-library/jest-dom'; import { axe } from 'jest-axe'; -import { i18nSupport } from '../../../utilities/i18n_support'; +import { i18nSupport } from '../../../__support__/i18n'; import { FollowUsers } from '../FollowUsers'; global.fetch = fetch; diff --git a/app/views/articles/index.html.erb b/app/views/articles/index.html.erb index 56932cead..10eb2150e 100644 --- a/app/views/articles/index.html.erb +++ b/app/views/articles/index.html.erb @@ -89,8 +89,6 @@ - <%= render "articles/reaction_category_resources" %> - <% if user_signed_in? %>
<% else %> diff --git a/app/views/collections/show.html.erb b/app/views/collections/show.html.erb index 3a2016094..566a1c266 100644 --- a/app/views/collections/show.html.erb +++ b/app/views/collections/show.html.erb @@ -11,8 +11,6 @@ <%= link_to t("views.series.list.back", user: @user.name), user_series_path(@user.username) %> - <%= render "articles/reaction_category_resources" %> -
<% if @articles.present? %> diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 4492ab813..47e2ddbc2 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -110,6 +110,7 @@ <%= render "layouts/signup_modal" unless user_signed_in? %> <% end %>
+ <%= render "articles/reaction_category_resources" %> <% end %> diff --git a/app/views/organizations/_main_feed.html.erb b/app/views/organizations/_main_feed.html.erb index d2f9d027f..e3254a8bf 100644 --- a/app/views/organizations/_main_feed.html.erb +++ b/app/views/organizations/_main_feed.html.erb @@ -1,5 +1,3 @@ -<%= render "articles/reaction_category_resources" %> -
<%= render partial: "articles/single_story", collection: @stories, as: :story, locals: { featured: false } %> <% if @stories.size > 1 %> diff --git a/app/views/stories/articles_search/index.html.erb b/app/views/stories/articles_search/index.html.erb index 9c6f35648..23a04d777 100644 --- a/app/views/stories/articles_search/index.html.erb +++ b/app/views/stories/articles_search/index.html.erb @@ -51,8 +51,6 @@ <%= render "stories/articles_search/nav_menu" %>
- <%= render "articles/reaction_category_resources" %> -
diff --git a/app/views/stories/tagged_articles/_main_feed.html.erb b/app/views/stories/tagged_articles/_main_feed.html.erb index f5c66ba41..765e24ce9 100644 --- a/app/views/stories/tagged_articles/_main_feed.html.erb +++ b/app/views/stories/tagged_articles/_main_feed.html.erb @@ -1,5 +1,3 @@ -<%= render "articles/reaction_category_resources" %> - <%= render partial: "articles/single_story", collection: @stories, as: :story, locals: { featured: false } %> <% if @stories.size > 1 %>
diff --git a/app/views/users/_main_feed.html.erb b/app/views/users/_main_feed.html.erb index 830e18599..c1f3e5263 100644 --- a/app/views/users/_main_feed.html.erb +++ b/app/views/users/_main_feed.html.erb @@ -1,5 +1,3 @@ -<%= render "articles/reaction_category_resources" %> - <% if @pinned_stories.present? %>
diff --git a/config/reactions.yml b/config/reactions.yml index 5551bda05..0cbb63377 100644 --- a/config/reactions.yml +++ b/config/reactions.yml @@ -9,12 +9,15 @@ unicorn: icon: multi-unicorn exploding_head: position: 3 + name: Exploding Head icon: exploding-head raised_hands: position: 4 + name: Raised Hands icon: raised-hands fire: position: 5 + name: Fire icon: fire readinglist: position: 5 diff --git a/testSetup.js b/testSetup.js index 72f98042b..da6c6f959 100644 --- a/testSetup.js +++ b/testSetup.js @@ -27,6 +27,9 @@ expect.extend({ * `undefined`, which makes it unsuitable for testing if a value exists as the * value might be `null` (which would pass a `toBeDefined` check). * + * Additionally this matcher will fail if its subject is a Promise, to remind + * the caller to await their Promises. + * * @param {any} subject The subject of the matcher * @returns */ @@ -38,6 +41,18 @@ expect.extend({ }; } + // This only works with JS native Promises, but that's what Jest and Testing + // Library produce, so that's a reasonable tradeoff + if (subject instanceof Promise) { + return { + pass: false, + message: () => + `Received a Promise (${this.utils.printReceived( + subject, + )}) instead of a value`, + }; + } + return { pass: true, message: () =>