From 30c04855079a723f1577658b71fe9ca1d6fdc2f3 Mon Sep 17 00:00:00 2001 From: Joshua Wehner Date: Mon, 23 Jan 2023 16:00:50 +0100 Subject: [PATCH] Frontend for multiple reactions (v1) (#18808) * Remove extraneous comment (see 9361d2 and 5c18f8) * Flexible, multiple reaction types * Fix reaction counts * Re-use svg for active state for now * Update yml, update spec * Possible temp fix for failing tests * Colorize reaction icon svgs * Reaction engagement above post title * Index reactions engagements (for logged-out) * Maybe readinglist is special * Try using crayons' dropdown as a drawer? * readinglist isn't really public now * feat: update the styles for the reaction drawer * Grey background highlight, turn off border/shadow * Read our feature flag docs, saw this was recommended * Missed fifth emoji: party/tada * Fix JS test errors * Update test with tada * Suppress flashing engagements when no public reactions * Liberate jump-to-comments from unicorn replacement * 'Add reaction' on tooltip * Don't show reaction emoji on index unless it's been used * rubocop * Update heart+ total count when toggling * Do not include 'readinglist' in drawer/public counts * Fix semi-public readinglist so that icon is badged for current user * Tweak heart-plus svg * Style tweak: border on active reaction * Show reacted icon on drawer trigger for 1.5 sec * Tweak styles for engagements bar * Style tweaks for multiple engagements (#index) * Trying to get size working through crayons/inline_svg * Sparkle hearts * Restore unicorn * Make heart-plus-active work when user has an active reaction * Try 'hoverdown' a dropdown that activates with hover * Long touch? * Tap *outside* the drawer to close * Mobile reaction drawer is also supposed to be columns * More reaction count cleanup * Final emoticons maybe? * Fix reaction bug when feature disabled * Remove readinglist from public reaction counts * Update specs for new reaction categories * Shuffle makes specs flaky * Order does not matter * rubocop * Update to preserve readinglist analytics * Shuffle makes specs flaky * Fix flickering images, remove icon highlight for now * Don't update total for readinglist * reactions_by_user_id * Try renaming this observer function * Try unid ids for SVGs * Remove local test file * Simplistic test for the unique svg transform * Fix javascript for current SVG * Signifcant string literals in this case, rubocop * Use the right expected output Co-authored-by: Ridhwana --- app/assets/config/manifest.js | 2 + app/assets/images/exploding-head.svg | 46 +++++++ app/assets/images/fire.svg | 14 ++ app/assets/images/heart-plus-active.svg | 11 ++ app/assets/images/heart-plus.svg | 3 + app/assets/images/lightbulb.svg | 0 app/assets/images/multi-unicorn.svg | 60 +++++++++ app/assets/images/raised-hands.svg | 26 ++++ app/assets/images/sparkle-heart.svg | 12 ++ app/assets/images/twemoji/raised_hands.svg | 7 + app/assets/images/twemoji/rocket.svg | 8 ++ app/assets/images/twemoji/sparkle-heart.svg | 4 + app/assets/images/twemoji/tada.svg | 17 +++ .../initializeArticleReactions.js | 122 ++++++++++++++++-- .../javascripts/utilities/isTouchDevice.js | 10 ++ .../utilities/watchForLongTouch.js | 40 ++++++ app/assets/stylesheets/config/_variables.scss | 1 + app/assets/stylesheets/views/article.scss | 121 +++++++++++++++++ app/controllers/reactions_controller.rb | 29 +++-- app/helpers/crayons_helper.rb | 5 +- app/javascript/packs/articlePage.jsx | 14 ++ app/javascript/topNavigation/utilities.js | 13 +- app/models/concerns/reactable.rb | 4 + app/models/reaction.rb | 36 +++++- app/models/reaction_category.rb | 4 + app/services/analytics_service.rb | 2 +- app/views/articles/_actions.html.erb | 46 +------ .../articles/_multiple_engagements.html.erb | 10 ++ .../articles/_multiple_reactions.html.erb | 61 +++++++++ .../articles/_multireaction_button.html.erb | 23 ++++ .../articles/_original_reactions.html.erb | 44 +++++++ app/views/articles/_sidebar.html.erb | 1 - app/views/articles/_single_story.html.erb | 29 ++++- app/views/articles/show.html.erb | 2 + config/initializers/inline_svg.rb | 7 + config/locales/views/reactions/en.yml | 7 + config/locales/views/reactions/fr.yml | 9 ++ config/reactions.yml | 23 +++- ...148_add_multiple_reactions_feature_flag.rb | 7 + lib/unique_svg_transform.rb | 35 +++++ spec/fixtures/files/unicorn-unique.svg | 61 +++++++++ spec/fixtures/files/unicorn.svg | 60 +++++++++ ...dd_multiple_reactions_feature_flag_spec.rb | 15 +++ spec/lib/unique_svg_transform_spec.rb | 15 +++ spec/models/article_spec.rb | 16 +++ spec/models/reaction_category_spec.rb | 9 +- spec/models/reaction_spec.rb | 2 +- spec/requests/notifications_spec.rb | 12 +- spec/requests/reactions_spec.rb | 4 +- 49 files changed, 999 insertions(+), 110 deletions(-) create mode 100644 app/assets/images/exploding-head.svg create mode 100644 app/assets/images/fire.svg create mode 100644 app/assets/images/heart-plus-active.svg create mode 100644 app/assets/images/heart-plus.svg create mode 100644 app/assets/images/lightbulb.svg create mode 100644 app/assets/images/multi-unicorn.svg create mode 100644 app/assets/images/raised-hands.svg create mode 100644 app/assets/images/sparkle-heart.svg create mode 100644 app/assets/images/twemoji/raised_hands.svg create mode 100644 app/assets/images/twemoji/rocket.svg create mode 100644 app/assets/images/twemoji/sparkle-heart.svg create mode 100644 app/assets/images/twemoji/tada.svg create mode 100644 app/assets/javascripts/utilities/isTouchDevice.js create mode 100644 app/assets/javascripts/utilities/watchForLongTouch.js create mode 100644 app/views/articles/_multiple_engagements.html.erb create mode 100644 app/views/articles/_multiple_reactions.html.erb create mode 100644 app/views/articles/_multireaction_button.html.erb create mode 100644 app/views/articles/_original_reactions.html.erb create mode 100644 config/initializers/inline_svg.rb create mode 100644 lib/data_update_scripts/20221220131148_add_multiple_reactions_feature_flag.rb create mode 100644 lib/unique_svg_transform.rb create mode 100644 spec/fixtures/files/unicorn-unique.svg create mode 100644 spec/fixtures/files/unicorn.svg create mode 100644 spec/lib/data_update_scripts/add_multiple_reactions_feature_flag_spec.rb create mode 100644 spec/lib/unique_svg_transform_spec.rb diff --git a/app/assets/config/manifest.js b/app/assets/config/manifest.js index 5ee822c3c..56e5d8fef 100644 --- a/app/assets/config/manifest.js +++ b/app/assets/config/manifest.js @@ -4,3 +4,5 @@ //= link_directory ../stylesheets .css //= link s3_direct_upload.js //= link katex.css +//= link icon.png +//= link loading-ellipsis.svg diff --git a/app/assets/images/exploding-head.svg b/app/assets/images/exploding-head.svg new file mode 100644 index 000000000..12f96c5c0 --- /dev/null +++ b/app/assets/images/exploding-head.svg @@ -0,0 +1,46 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/app/assets/images/fire.svg b/app/assets/images/fire.svg new file mode 100644 index 000000000..d198aba73 --- /dev/null +++ b/app/assets/images/fire.svg @@ -0,0 +1,14 @@ + + + + + + + + + + + + + + diff --git a/app/assets/images/heart-plus-active.svg b/app/assets/images/heart-plus-active.svg new file mode 100644 index 000000000..6c1e20471 --- /dev/null +++ b/app/assets/images/heart-plus-active.svg @@ -0,0 +1,11 @@ + + + + + + + + + + + diff --git a/app/assets/images/heart-plus.svg b/app/assets/images/heart-plus.svg new file mode 100644 index 000000000..d113aa8a8 --- /dev/null +++ b/app/assets/images/heart-plus.svg @@ -0,0 +1,3 @@ + + + diff --git a/app/assets/images/lightbulb.svg b/app/assets/images/lightbulb.svg new file mode 100644 index 000000000..e69de29bb diff --git a/app/assets/images/multi-unicorn.svg b/app/assets/images/multi-unicorn.svg new file mode 100644 index 000000000..ada706cd2 --- /dev/null +++ b/app/assets/images/multi-unicorn.svg @@ -0,0 +1,60 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/app/assets/images/raised-hands.svg b/app/assets/images/raised-hands.svg new file mode 100644 index 000000000..ae8d6254a --- /dev/null +++ b/app/assets/images/raised-hands.svg @@ -0,0 +1,26 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/app/assets/images/sparkle-heart.svg b/app/assets/images/sparkle-heart.svg new file mode 100644 index 000000000..43293cf70 --- /dev/null +++ b/app/assets/images/sparkle-heart.svg @@ -0,0 +1,12 @@ + + + + + + + + + + + + diff --git a/app/assets/images/twemoji/raised_hands.svg b/app/assets/images/twemoji/raised_hands.svg new file mode 100644 index 000000000..5c87b88d3 --- /dev/null +++ b/app/assets/images/twemoji/raised_hands.svg @@ -0,0 +1,7 @@ + + + + + + + diff --git a/app/assets/images/twemoji/rocket.svg b/app/assets/images/twemoji/rocket.svg new file mode 100644 index 000000000..f998df8a8 --- /dev/null +++ b/app/assets/images/twemoji/rocket.svg @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/app/assets/images/twemoji/sparkle-heart.svg b/app/assets/images/twemoji/sparkle-heart.svg new file mode 100644 index 000000000..3773c22bc --- /dev/null +++ b/app/assets/images/twemoji/sparkle-heart.svg @@ -0,0 +1,4 @@ + + + + diff --git a/app/assets/images/twemoji/tada.svg b/app/assets/images/twemoji/tada.svg new file mode 100644 index 000000000..7dbe5b4a8 --- /dev/null +++ b/app/assets/images/twemoji/tada.svg @@ -0,0 +1,17 @@ + + + + + + + + + + + + + + + + + diff --git a/app/assets/javascripts/initializers/initializeArticleReactions.js b/app/assets/javascripts/initializers/initializeArticleReactions.js index 31128b894..bc23fa9b7 100644 --- a/app/assets/javascripts/initializers/initializeArticleReactions.js +++ b/app/assets/javascripts/initializers/initializeArticleReactions.js @@ -1,19 +1,29 @@ -/* global sendHapticMessage, showLoginModal, showModalAfterError */ +/* global sendHapticMessage, showLoginModal, showModalAfterError, isTouchDevice, watchForLongTouch */ // Set reaction count to correct number function setReactionCount(reactionName, newCount) { - var reactionClassList = document.getElementById( + var reactionButtons = document.getElementById( 'reaction-butt-' + reactionName, ).classList; - var reactionNumber = document.getElementById( + var reactionButtonCounter = document.getElementById( 'reaction-number-' + reactionName, ); + var reactionEngagementCounter = document.getElementById( + 'reaction_engagement_' + reactionName + '_count', + ); if (newCount > 0) { - reactionClassList.add('activated'); - reactionNumber.textContent = newCount; + reactionButtons.add('activated'); + reactionButtonCounter.textContent = newCount; + if (reactionEngagementCounter) { + reactionEngagementCounter.parentElement.classList.remove('hidden'); + reactionEngagementCounter.textContent = newCount; + } } else { - reactionClassList.remove('activated'); - reactionNumber.textContent = '0'; + reactionButtons.remove('activated'); + reactionButtonCounter.textContent = '0'; + if (reactionEngagementCounter) { + reactionEngagementCounter.parentElement.classList.add('hidden'); + } } } @@ -23,6 +33,41 @@ function showUserReaction(reactionName, animatedClass) { ); reactionButton.classList.add('user-activated', animatedClass); reactionButton.setAttribute('aria-pressed', 'true'); + + const reactionDrawerButton = document.getElementById( + 'reaction-drawer-trigger', + ); + + // the rest only applies to multiple reactions feature flag + if (!reactionDrawerButton) { + return; + } + + if (reactionDrawerButton && reactionName !== 'readinglist') { + reactionDrawerButton.classList.add('user-activated', 'user-animated'); + } + + if (animatedClass == 'user-animated') { + const activeIcon = reactionButton.querySelector( + '.crayons-reaction__icon--active svg', + ); + + if (activeIcon) { + const activeDrawerIcon = reactionDrawerButton.querySelector( + '.crayons-reaction__icon--active img', + ); + + reactionDrawerButton.originalIcon = activeDrawerIcon.outerHTML; + activeDrawerIcon.outerHTML = activeIcon.outerHTML; + + setTimeout(function () { + document + .getElementById('reaction-drawer-trigger') + .querySelector('.crayons-reaction__icon--active svg').outerHTML = + reactionDrawerButton.originalIcon; + }, 1500); + } + } } function hideUserReaction(reactionName) { @@ -49,15 +94,25 @@ function getNumReactions(reactionName) { } function reactToArticle(articleId, reaction) { + var reactionTotalCount = document.getElementById('reaction_total_count'); + + var isReadingList = reaction === 'readinglist'; + // Visually toggle the reaction function toggleReaction() { var currentNum = getNumReactions(reaction); if (hasUserReacted(reaction)) { hideUserReaction(reaction); setReactionCount(reaction, currentNum - 1); + if (reactionTotalCount && !isReadingList) { + reactionTotalCount.innerText = Number(reactionTotalCount.innerText) - 1; + } } else { showUserReaction(reaction, 'user-animated'); setReactionCount(reaction, currentNum + 1); + if (reactionTotalCount && !isReadingList) { + reactionTotalCount.innerText = Number(reactionTotalCount.innerText) + 1; + } } } var userStatus = document.body.getAttribute('data-user-status'); @@ -153,9 +208,47 @@ function requestReactionCounts(articleId) { ajaxReq.send(); } +function openDrawerOnHover() { + var timer; + const drawerTrigger = document.getElementById('reaction-drawer-trigger'); + if (!drawerTrigger) { + return; + } + + drawerTrigger.addEventListener('click', function (event) { + event.preventDefault(); + }); + + if (isTouchDevice()) { + watchForLongTouch(drawerTrigger); + drawerTrigger.addEventListener('longTouch', function () { + drawerTrigger.parentElement.classList.add('open'); + }); + document.addEventListener('touchstart', function (event) { + if (!drawerTrigger.parentElement.contains(event.target)) { + drawerTrigger.parentElement.classList.remove('open'); + } + }); + } else { + document.querySelectorAll('.hoverdown').forEach(function (el) { + el.addEventListener('mouseover', function (event) { + this.classList.add('open'); + clearTimeout(timer); + }); + el.addEventListener('mouseout', function (event) { + timer = setTimeout(function (event) { + document.querySelector('.hoverdown.open').classList.remove('open'); + }, 1000); + }); + }); + } +} + function initializeArticleReactions() { setCollectionFunctionality(); + openDrawerOnHover(); + setTimeout(() => { var reactionButts = document.getElementsByClassName('crayons-reaction'); @@ -167,6 +260,9 @@ function initializeArticleReactions() { requestReactionCounts(articleId); for (var i = 0; i < reactionButts.length; i += 1) { + if (reactionButts[i].classList.contains('pseudo-reaction')) { + continue; + } reactionButts[i].onclick = function addReactionOnClick(e) { reactToArticle(articleId, this.dataset.category); }; @@ -174,13 +270,15 @@ function initializeArticleReactions() { } var jumpToCommentsButt = document.getElementById('reaction-butt-comment'); - var commentsSection = document.getElementById("comments"); - if (document.getElementById('article-body') && commentsSection && jumpToCommentsButt) { - + var commentsSection = document.getElementById('comments'); + if ( + document.getElementById('article-body') && + commentsSection && + jumpToCommentsButt + ) { jumpToCommentsButt.onclick = function jumpToComments(e) { - commentsSection.scrollIntoView({behavior: "smooth"}); + commentsSection.scrollIntoView({ behavior: 'smooth' }); }; - } }, 3); } diff --git a/app/assets/javascripts/utilities/isTouchDevice.js b/app/assets/javascripts/utilities/isTouchDevice.js new file mode 100644 index 000000000..853fa21ce --- /dev/null +++ b/app/assets/javascripts/utilities/isTouchDevice.js @@ -0,0 +1,10 @@ +/** + * Determines whether or not a device is a touch device. + * + * @returns true if a touch device, otherwise false. + */ +function isTouchDevice() { + return /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini|DEV-Native-ios/i.test( + navigator.userAgent, + ); +} diff --git a/app/assets/javascripts/utilities/watchForLongTouch.js b/app/assets/javascripts/utilities/watchForLongTouch.js new file mode 100644 index 000000000..3427608d3 --- /dev/null +++ b/app/assets/javascripts/utilities/watchForLongTouch.js @@ -0,0 +1,40 @@ +// Watch @param element for a "long touch" (800ms) and dispatch a custom "longTouch" event, +// which you are presumably listening for elsewhere. Note that this also disables context menu +// for the element, as otherwise device will default to opening a context menu instead. +function watchForLongTouch(element) { + const longTouchEvent = new Event('longTouch'); + const minDuration = 800; + let timer; + + // In the event of a long touch, dispatch the "longTouch" event + const dispatcher = function () { + element.dispatchEvent(longTouchEvent); + }; + + // Observe "touchstart", if it's still going after a duration, dispatch "longTouch" + const onTouchStart = function (event) { + if (!timer) { + timer = setTimeout(dispatcher, minDuration); + } + }; + + // Observe "touchend", if they've stopped touching, it won't become a longTouch anytime soon + const onTouchEnd = function () { + if (timer) { + clearTimeout(timer); + timer = false; + } + }; + + if (element) { + // Disable contextmenu on device, otherwise they'll get a device OS menu instead of ours + element.oncontextmenu = function (event) { + event.preventDefault(); + event.stopPropagation(); // not necessary in my case, could leave in case stopImmediateProp isn't available? + event.stopImmediatePropagation(); + return false; + }; + element.addEventListener('touchstart', onTouchStart); + element.addEventListener('touchend', onTouchEnd); + } +} diff --git a/app/assets/stylesheets/config/_variables.scss b/app/assets/stylesheets/config/_variables.scss index 87a361778..11bdab0ed 100644 --- a/app/assets/stylesheets/config/_variables.scss +++ b/app/assets/stylesheets/config/_variables.scss @@ -36,6 +36,7 @@ --radius: 0.375rem; --radius-large: 0.75rem; + --radius-xl: 2rem; // 1. this is a trick for conditional border-radius: https://ishadeed.com/article/conditional-border-radius/ // 2. intentionally using uppercase Max and Min because SCSS have issues otherwise... --radius-auto: Max(0px, Min(var(--radius), calc((100vw - 4px - 100%) * 9999))) / diff --git a/app/assets/stylesheets/views/article.scss b/app/assets/stylesheets/views/article.scss index c36236d7d..b7641ad95 100644 --- a/app/assets/stylesheets/views/article.scss +++ b/app/assets/stylesheets/views/article.scss @@ -241,6 +241,23 @@ } } +// This overrides some of the styles for the temporarily feature-flaged +.reaction-drawer .crayons-reaction { + border: 1px solid var(--card-bg); + flex-direction: column; + + &.user-activated { + background-color: var(--body-bg); + border: 1px solid #d4d4d4; + border-radius: 20px; + + .crayons-reaction__icon--active { + border: 0; + box-shadow: none; + } + } +} + .user-animated { .crayons-reaction__icon { position: relative; @@ -349,3 +366,107 @@ color: var(--card-color-tertiary); } } + +// TODO: The drawer__outer contains the trigger button and the drawer itself, +// enabling hover, however this then requires the __outer wrapping div to be +// styled similarly to the other action buttons in the sidebar, which we can +// clean up significantly *after* the multiple_reactions feature flag is +// fully rolled-out. +.reaction-drawer__outer { + border: none; + padding: 0; + background: transparent; + display: inline-flex; + flex-direction: row; + align-items: center; + + @media (min-width: $breakpoint-m) { + flex-direction: column; + } + + &.open .reaction-drawer { + display: block; + } + + .hidden { + display: none; + } +} + +.reaction-drawer { + min-width: auto; + border-radius: 32px; + position: absolute; + top: -80px; + left: 15px; + display: none; // Initially hidden + + // Copying from Dropdown for now + padding: var(--su-2); + min-width: 250px; + margin-top: var(--su-1); + z-index: var(--z-dropdown); + background: var(--card-bg); + color: var(--card-color); + box-shadow: var(--shadow-1); + border-radius: var(--radius); + + flex-direction: column; + + @media (min-width: $breakpoint-s) { + max-width: 360px; + width: max-content; + top: -5px; + left: 55px; + } + + .crayons-reaction__count { + min-width: auto; + margin-left: 0; + display: block; + } +} + +.reaction-drawer__outer:hover .reaction-drawer { + @media (min-width: $breakpoint-m) { + display: block; + } +} + +.reaction-drawer__container { + display: flex; + flex-direction: row; + gap: var(--su-3); +} + +.multiple_reactions_engagement { + padding: 0.5em 0; + + span { + margin-right: 1em; + } + svg { + display: inline-block; + } +} + +.multiple_reactions_aggregate { + padding-right: 1em; + + .crayons_icon_container { + display: inline-block; + background-color: var(--body-bg); + border-radius: 15px; + margin-right: -0.75em; + width: 24px; + height: 24px; + } + + svg { + margin: 3px 3px; + } + + .aggregate_reactions_counter { + margin-left: 0.75em; + } +} diff --git a/app/controllers/reactions_controller.rb b/app/controllers/reactions_controller.rb index 84e1cdeb6..5741f7929 100644 --- a/app/controllers/reactions_controller.rb +++ b/app/controllers/reactions_controller.rb @@ -12,16 +12,8 @@ class ReactionsController < ApplicationController if params[:article_id] id = params[:article_id] - reactions = if session_current_user_id - Reaction.public_category - .where( - reactable_id: id, - reactable_type: "Article", - user_id: session_current_user_id, - ) - else - Reaction.none - end + reactions = reactions_by_user_id(session_current_user_id) if session_current_user_id + reactions ||= Reaction.none result = { article_reaction_counts: Reaction.count_for_article(id) } else @@ -87,6 +79,23 @@ class ReactionsController < ApplicationController authorize(Reaction, policy_query) end + def reactions_by_user_id(user_id = session_current_user_id) + id = params[:article_id] + + public_reactions = Reaction.public_category.where( + reactable_id: id, + reactable_type: "Article", + user_id: user_id, + ) + readinglist = Reaction.where( + reactable_id: id, + reactable_type: "Article", + category: "readinglist", user_id: user_id + ) + + public_reactions + readinglist + end + # TODO: should this move to toggle service? refactor? def remove_count_cache_key return unless params[:reactable_type] == "Article" diff --git a/app/helpers/crayons_helper.rb b/app/helpers/crayons_helper.rb index 364a4e2d0..66a117e6b 100644 --- a/app/helpers/crayons_helper.rb +++ b/app/helpers/crayons_helper.rb @@ -29,6 +29,9 @@ module CrayonsHelper opts.delete(:class), ].compact.join(" ") - inline_svg_tag(icon_name, aria: true, width: 24, height: 24, class: icon_class, **opts) + opts[:width] ||= 24 + opts[:height] ||= 24 + + inline_svg_tag(icon_name, aria: true, class: icon_class, **opts) end end diff --git a/app/javascript/packs/articlePage.jsx b/app/javascript/packs/articlePage.jsx index 75de943cd..27fa9bc0d 100644 --- a/app/javascript/packs/articlePage.jsx +++ b/app/javascript/packs/articlePage.jsx @@ -34,6 +34,20 @@ if (snackZone) { // eslint-disable-next-line no-restricted-globals top.addSnackbarItem = addSnackbarItem; +const multiReactionDrawerTrigger = document.getElementById( + 'reaction-drawer-trigger', +); + +if ( + multiReactionDrawerTrigger && + multiReactionDrawerTrigger.dataset.initialized !== 'true' +) { + initializeDropdown({ + triggerElementId: 'reaction-drawer-trigger', + dropdownContentId: 'reaction-drawer', + }); +} + // Dropdown accessibility function hideCopyLinkAnnouncerIfVisible() { document.getElementById('article-copy-link-announcer').hidden = true; diff --git a/app/javascript/topNavigation/utilities.js b/app/javascript/topNavigation/utilities.js index b6ac2e6be..304166384 100644 --- a/app/javascript/topNavigation/utilities.js +++ b/app/javascript/topNavigation/utilities.js @@ -1,3 +1,5 @@ +/* global isTouchDevice */ + function closeHeaderMenu(memberMenu, menuNavButton) { menuNavButton.setAttribute('aria-expanded', 'false'); memberMenu.classList.remove('desktop', 'showing'); @@ -28,17 +30,6 @@ function openHeaderMenu(memberMenu, menuNavButton) { })(); } -/** - * Determines whether or not a device is a touch device. - * - * @returns true if a touch device, otherwise false. - */ -export function isTouchDevice() { - return /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini|DEV-Native-ios/i.test( - navigator.userAgent, - ); -} - /** * Initializes the member navigation menu events. * diff --git a/app/models/concerns/reactable.rb b/app/models/concerns/reactable.rb index 5b33c91c0..6611d6857 100644 --- a/app/models/concerns/reactable.rb +++ b/app/models/concerns/reactable.rb @@ -8,4 +8,8 @@ module Reactable def sync_reactions_count update_column(:public_reactions_count, reactions.public_category.size) end + + def reaction_categories + reactions.distinct(:category).pluck(:category) + end end diff --git a/app/models/reaction.rb b/app/models/reaction.rb index aa446bd68..9578c426c 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -7,11 +7,16 @@ class Reaction < ApplicationRecord counter_culture :reactable, column_name: proc { |model| - ReactionCategory[model.category].visible_to_public? ? "public_reactions_count" : "reactions_count" + # After FeatureFlag :multiple_reactions, this could change to: + # ReactionCategory[model.category].visible_to_public? + public_reaction_types.include?(model.category.to_s) ? "public_reactions_count" : "reactions_count" } counter_culture :user - scope :public_category, -> { where(category: ReactionCategory.public.map(&:to_s)) } + scope :public_category, lambda { + categories = public_reaction_types + where(category: categories) + } # Be wary, this is all things on the reading list, but for an end # user they might only see readinglist items that are published. @@ -55,10 +60,8 @@ class Reaction < ApplicationRecord reactions = Reaction.where(reactable_id: id, reactable_type: "Article") counts = reactions.group(:category).count - reaction_types = %w[like readinglist] - unless FeatureFlag.enabled?(:replace_unicorn_with_jump_to_comments) - reaction_types << "unicorn" - end + reaction_types = public_reaction_types + reaction_types << "readinglist" unless public_reaction_types.include?("readinglist") reaction_types.map do |type| { category: type, count: counts.fetch(type, 0) } @@ -75,6 +78,27 @@ class Reaction < ApplicationRecord end end + def public_reaction_types + if FeatureFlag.enabled?(:multiple_reactions) + reaction_types = ReactionCategory.public.map(&:to_s) - ["readinglist"] + else + # used to include "readinglist" but that's not correct now, even without the feature flag + # we aren't going to re-process these, they will gradually correct over time + reaction_types = %w[like] + unless FeatureFlag.enabled?(:replace_unicorn_with_jump_to_comments) + reaction_types << "unicorn" + end + end + + reaction_types + end + + def for_analytics + reaction_types = public_reaction_types + reaction_types << "readinglist" unless public_reaction_types.include?("readinglist") + where(category: reaction_types) + end + # @param user [User] the user who might be spamming the system # @param threshold [Integer] the number of strikes before they are spam # @param include_user_profile [Boolean] do we include the user's profile as part of the "check diff --git a/app/models/reaction_category.rb b/app/models/reaction_category.rb index 87d779adc..608d40f03 100644 --- a/app/models/reaction_category.rb +++ b/app/models/reaction_category.rb @@ -13,6 +13,10 @@ class ReactionCategory list.filter_map { |category| category.slug if category.privileged? && category.negative? } end + def for_view + list.sort_by(&:position).filter_map { |category| category if category.visible_to_public? } + end + def public list.sort_by(&:position).filter_map { |category| category.slug if category.visible_to_public? } end diff --git a/app/services/analytics_service.rb b/app/services/analytics_service.rb index 98f1e933f..cdf2bda07 100644 --- a/app/services/analytics_service.rb +++ b/app/services/analytics_service.rb @@ -91,7 +91,7 @@ class AnalyticsService .where("score > 0") @follow_data = Follow .where(followable_type: user_or_org.class.name, followable_id: user_or_org.id) - @reaction_data = Reaction.public_category + @reaction_data = Reaction.for_analytics .where(reactable_id: article_ids, reactable_type: "Article") @page_view_data = PageView.where(article_id: article_ids) diff --git a/app/views/articles/_actions.html.erb b/app/views/articles/_actions.html.erb index d38c70e4c..fcb5dd56b 100644 --- a/app/views/articles/_actions.html.erb +++ b/app/views/articles/_actions.html.erb @@ -1,51 +1,11 @@
<% if @article.published? %> - - <%= render partial: "articles/reaction_button", - locals: { - category: :like, - description: t("views.reactions.like.title"), - image_path: "heart.svg", - image_active_path: "heart-filled.svg", - aria_label: t("views.reactions.like.aria_label") - } %> - - <% if FeatureFlag.enabled?(:replace_unicorn_with_jump_to_comments) %> - + <% if FeatureFlag.enabled?(:multiple_reactions) %> + <%= render partial: "articles/multiple_reactions" %> <% else %> - <%= render partial: "articles/reaction_button", - locals: { - category: :unicorn, - description: t("views.reactions.unicorn.title"), - image_path: "unicorn.svg", - image_active_path: "unicorn-filled.svg", - aria_label: t("views.reactions.unicorn.aria_label") - } %> + <%= render partial: "articles/original_reactions" %> <% end %> - - <%= render partial: "articles/reaction_button", - locals: { - category: :readinglist, - description: t("views.reactions.readingList.title"), - image_path: "save.svg", - image_active_path: "save-filled.svg", - aria_label: t("views.reactions.readingList.aria_label") - } %> <% end %>
diff --git a/app/views/articles/_multiple_engagements.html.erb b/app/views/articles/_multiple_engagements.html.erb new file mode 100644 index 000000000..1deec8cd2 --- /dev/null +++ b/app/views/articles/_multiple_engagements.html.erb @@ -0,0 +1,10 @@ +
+ <% ReactionCategory.for_view.each do |reaction_type| %> + + <% end %> + +   +
diff --git a/app/views/articles/_multiple_reactions.html.erb b/app/views/articles/_multiple_reactions.html.erb new file mode 100644 index 000000000..7e9238b69 --- /dev/null +++ b/app/views/articles/_multiple_reactions.html.erb @@ -0,0 +1,61 @@ + +
+ + + +
+ + + +<%= render partial: "articles/reaction_button", + locals: { + category: :readinglist, + description: t("views.reactions.readingList.title"), + image_path: "save.svg", + image_active_path: "save-filled.svg", + aria_label: t("views.reactions.readingList.aria_label") + } %> diff --git a/app/views/articles/_multireaction_button.html.erb b/app/views/articles/_multireaction_button.html.erb new file mode 100644 index 000000000..85331ae90 --- /dev/null +++ b/app/views/articles/_multireaction_button.html.erb @@ -0,0 +1,23 @@ + diff --git a/app/views/articles/_original_reactions.html.erb b/app/views/articles/_original_reactions.html.erb new file mode 100644 index 000000000..daf38c136 --- /dev/null +++ b/app/views/articles/_original_reactions.html.erb @@ -0,0 +1,44 @@ +<%= render partial: "articles/reaction_button", + locals: { + category: :like, + description: t("views.reactions.like.title"), + image_path: "heart.svg", + image_active_path: "heart-filled.svg", + aria_label: t("views.reactions.like.aria_label") + } %> + +<% if FeatureFlag.enabled?(:replace_unicorn_with_jump_to_comments) %> + +<% else %> + <%= render partial: "articles/reaction_button", + locals: { + category: :unicorn, + description: t("views.reactions.unicorn.title"), + image_path: "unicorn.svg", + image_active_path: "unicorn-filled.svg", + aria_label: t("views.reactions.unicorn.aria_label") + } %> +<% end %> + +<%= render partial: "articles/reaction_button", + locals: { + category: :readinglist, + description: t("views.reactions.readingList.title"), + image_path: "save.svg", + image_active_path: "save-filled.svg", + aria_label: t("views.reactions.readingList.aria_label") + } %> diff --git a/app/views/articles/_sidebar.html.erb b/app/views/articles/_sidebar.html.erb index 032313ec0..c1211bc8a 100644 --- a/app/views/articles/_sidebar.html.erb +++ b/app/views/articles/_sidebar.html.erb @@ -4,7 +4,6 @@ <%= render partial: "shared/auth_widget", locals: { tracking_id: "ca_left_sidebar_home_page", source: "left_sidebar" } unless user_signed_in? %> <%= render partial: "layouts/main_nav", locals: { context: "sidebar" } %> <%= render "layouts/sidebar_tags" %> - <%# the pattern .present?/.each has the advantage of issuing only a single SQL query to load objects in memory %> <% end %> <% cache("display-area-left-#{rand(5)}-#{user_signed_in?}", expires_in: 5.minutes) do %> <% @left_sidebar_ad = DisplayAd.for_display("sidebar_left", user_signed_in?) %> diff --git a/app/views/articles/_single_story.html.erb b/app/views/articles/_single_story.html.erb index 6b428976b..e32024023 100644 --- a/app/views/articles/_single_story.html.erb +++ b/app/views/articles/_single_story.html.erb @@ -120,13 +120,28 @@
<% if story.public_reactions_count > 0 %> - "> - <%= crayons_icon_tag("small-heart", title: t("views.reactions.summary.title")) %> - <%= t("views.reactions.summary.count_html", - count: story.public_reactions_count, - start: tag("span", { class: %w[hidden s:inline] }, true), - end: "".html_safe) %> - + <% if FeatureFlag.enabled?(:multiple_reactions) %> +
+ <% ReactionCategory.for_view.each do |reaction_type| %> + <% next unless story.reaction_categories.include?(reaction_type.slug.to_s) %> + + <%= crayons_icon_tag(reaction_type.icon, width: "18px", height: "18px", native: true, aria_hidden: true) %> + + <% end %> + <%= t("views.reactions.summary.count_html", + count: story.public_reactions_count, + start: tag("span", { class: %w[hidden s:inline] }, true), + end: "".html_safe) %> +
+ <% else %> + "> + <%= crayons_icon_tag("small-heart", title: t("views.reactions.summary.title")) %> + <%= t("views.reactions.summary.count_html", + count: story.public_reactions_count, + start: tag("span", { class: %w[hidden s:inline] }, true), + end: "".html_safe) %> + + <% end %> <% end %> <% if story.comments_count > 0 %> "> diff --git a/app/views/articles/show.html.erb b/app/views/articles/show.html.erb index beee2641e..9f2e1a645 100644 --- a/app/views/articles/show.html.erb +++ b/app/views/articles/show.html.erb @@ -150,6 +150,8 @@
+ <%= render "articles/multiple_engagements" if FeatureFlag.enabled?(:multiple_reactions) && @article.public_reactions_count? %> +

<% if @article.search_optimized_title_preamble.present? && !user_signed_in? %> <%= @article.search_optimized_title_preamble %> diff --git a/config/initializers/inline_svg.rb b/config/initializers/inline_svg.rb new file mode 100644 index 000000000..841d9ee77 --- /dev/null +++ b/config/initializers/inline_svg.rb @@ -0,0 +1,7 @@ +require "unique_svg_transform" + +InlineSvg.configure do |config| + # NOTE: There is currently a bug in the inline_svg gem where custom attributes + # are de-facto required in order for a custom transformation to be activated. + config.add_custom_transformation attribute: :force_unique, transform: UniqueSvgTransform +end diff --git a/config/locales/views/reactions/en.yml b/config/locales/views/reactions/en.yml index ec8b55b9c..411443260 100644 --- a/config/locales/views/reactions/en.yml +++ b/config/locales/views/reactions/en.yml @@ -11,6 +11,10 @@ en: thumbsdown: Thumbs down unicorn: Unicorn vomit: Flag + lightbulb: Lightbulb + raised_hands: Raised hands + rocket: Rocket + drawer_button: Add reaction summary: count_html: one: "%{count}%{start}\_reaction%{end}" @@ -29,3 +33,6 @@ en: aria_label: Add to reading list icon: Save button text: Save + readinglist: + title: Save + aria_label: Add to reading list diff --git a/config/locales/views/reactions/fr.yml b/config/locales/views/reactions/fr.yml index 4a4ca6800..3530947e4 100644 --- a/config/locales/views/reactions/fr.yml +++ b/config/locales/views/reactions/fr.yml @@ -11,6 +11,10 @@ fr: thumbsdown: Thumbs down unicorn: Unicorn vomit: Flag + lightbulb: Lightbulb + raised_hands: Raised hands + rocket: Rocket + drawer_button: Add reaction summary: count_html: one: "%{count}%{start}\_réaction%{end}" @@ -29,3 +33,8 @@ fr: aria_label: Add to reading list icon: Save button text: Save + readinglist: + title: Sauver + aria_label: Add to reading list + icon: Save button + text: Save diff --git a/config/reactions.yml b/config/reactions.yml index 41d1a564b..1c51ce63e 100644 --- a/config/reactions.yml +++ b/config/reactions.yml @@ -1,13 +1,25 @@ --- like: position: 1 - icon: later -readinglist: - position: 2 - icon: later + name: Like + icon: sparkle-heart unicorn: + position: 2 + name: Unicorn + icon: multi-unicorn +exploding-head: position: 3 - icon: later + icon: exploding-head +raised_hands: + position: 4 + icon: raised-hands +fire: + position: 5 + icon: fire +readinglist: + position: 5 + icon: save + published: false thumbsup: privileged: true score: 5.0 @@ -16,6 +28,7 @@ thumbsdown: score: -10.0 vomit: privileged: true + name: Vomit score: -50.0 thinking: published: false diff --git a/lib/data_update_scripts/20221220131148_add_multiple_reactions_feature_flag.rb b/lib/data_update_scripts/20221220131148_add_multiple_reactions_feature_flag.rb new file mode 100644 index 000000000..014d70c8a --- /dev/null +++ b/lib/data_update_scripts/20221220131148_add_multiple_reactions_feature_flag.rb @@ -0,0 +1,7 @@ +module DataUpdateScripts + class AddMultipleReactionsFeatureFlag + def run + FeatureFlag.add :multiple_reactions + end + end +end diff --git a/lib/unique_svg_transform.rb b/lib/unique_svg_transform.rb new file mode 100644 index 000000000..c6e19be31 --- /dev/null +++ b/lib/unique_svg_transform.rb @@ -0,0 +1,35 @@ +require "securerandom" + +# rubocop:disable Style/StringLiterals + +class UniqueSvgTransform < InlineSvg::CustomTransformation + def transform(doc, uniq_str = SecureRandom.hex[0..6]) + # nodes with id attributes + doc.xpath('//*[@id]').each do |node| + node_id = node.get_attribute 'id' + node.set_attribute 'id', [node_id, uniq_str].join('-') + end + + # nodes with id references + doc.xpath('//*[@href]').each do |node| + node_href = node.get_attribute 'href' + node.set_attribute 'href', [node_href, uniq_str].join('-') + end + + # nodes with fill references + doc.xpath('//*[@fill]').each do |node| + node_url = node.get_attribute 'fill' + if (md = /url\((.+)\)/.match node_url) + id = md[1] + end + next unless id + + new_url = "url(#{id}-#{uniq_str})" + node.set_attribute 'fill', new_url + end + + doc + end +end + +# rubocop:enable Style/StringLiterals diff --git a/spec/fixtures/files/unicorn-unique.svg b/spec/fixtures/files/unicorn-unique.svg new file mode 100644 index 000000000..07b94ce0f --- /dev/null +++ b/spec/fixtures/files/unicorn-unique.svg @@ -0,0 +1,61 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/spec/fixtures/files/unicorn.svg b/spec/fixtures/files/unicorn.svg new file mode 100644 index 000000000..117f84cb7 --- /dev/null +++ b/spec/fixtures/files/unicorn.svg @@ -0,0 +1,60 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/spec/lib/data_update_scripts/add_multiple_reactions_feature_flag_spec.rb b/spec/lib/data_update_scripts/add_multiple_reactions_feature_flag_spec.rb new file mode 100644 index 000000000..679326361 --- /dev/null +++ b/spec/lib/data_update_scripts/add_multiple_reactions_feature_flag_spec.rb @@ -0,0 +1,15 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20221220131148_add_multiple_reactions_feature_flag.rb", +) + +describe DataUpdateScripts::AddMultipleReactionsFeatureFlag do + before do + allow(FeatureFlag).to receive(:add) + end + + it "adds the feature flag" do + described_class.new.run + expect(FeatureFlag).to have_received(:add).with(:multiple_reactions) + end +end diff --git a/spec/lib/unique_svg_transform_spec.rb b/spec/lib/unique_svg_transform_spec.rb new file mode 100644 index 000000000..82d3d214e --- /dev/null +++ b/spec/lib/unique_svg_transform_spec.rb @@ -0,0 +1,15 @@ +require "rails_helper" + +require "unique_svg_transform" + +RSpec.describe UniqueSvgTransform do + let(:input) { "#{fixture_path}/files/unicorn.svg" } + let(:expectation) { File.read("#{fixture_path}/files/unicorn-unique.svg") } + let(:pseudo_unique) { "87a63a" } + + it "transforms input doc into uniquely identified SVG" do + doc = Nokogiri::XML(File.open(input)) + output = described_class.new(true).transform(doc, pseudo_unique).to_xml + expect(output).to eq expectation + end +end diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 10a6b556e..46a694455 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -1341,4 +1341,20 @@ RSpec.describe Article do expect(another_article.errors.messages[:feed_source_url]).to include(error_message) end end + + describe "#reaction_categories reports unique associated reaction categories" do + before do + user2 = create(:user) + user2.add_role(:trusted) + + create(:reaction, reactable: article, category: "like") + create(:reaction, reactable: article, category: "like") + create(:reaction, reactable: article, category: "readinglist") + create(:reaction, reactable: article, category: "vomit", user: user2) + end + + it "reports accurately" do + expect(article.reaction_categories).to contain_exactly("like", "readinglist", "vomit") + end + end end diff --git a/spec/models/reaction_category_spec.rb b/spec/models/reaction_category_spec.rb index 76c2634a2..bde730586 100644 --- a/spec/models/reaction_category_spec.rb +++ b/spec/models/reaction_category_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe ReactionCategory, type: :model do +RSpec.describe ReactionCategory do let(:attributes_hash) do { "slug" => "lol", @@ -17,12 +17,13 @@ RSpec.describe ReactionCategory, type: :model do end it "lists all category slugs" do - expect(described_class.all_slugs).to contain_exactly(*%i[like unicorn readinglist hands thinking thumbsup - thumbsdown vomit]) + expect(described_class.all_slugs).to contain_exactly(*%i[exploding-head fire hands like + raised_hands readinglist thinking + thumbsdown thumbsup unicorn vomit]) end it "lists public categories" do - expect(described_class.public).to contain_exactly(*%i[like readinglist unicorn]) + expect(described_class.public).to contain_exactly(*%i[like unicorn raised_hands fire exploding-head]) end it "lists privileged categories" do diff --git a/spec/models/reaction_spec.rb b/spec/models/reaction_spec.rb index 0d1c840c4..ef64e472d 100644 --- a/spec/models/reaction_spec.rb +++ b/spec/models/reaction_spec.rb @@ -139,8 +139,8 @@ RSpec.describe Reaction do expected_result = [ { category: "like", count: 1 }, - { category: "readinglist", count: 0 }, { category: "unicorn", count: 1 }, + { category: "readinglist", count: 0 }, ] expect(described_class.count_for_article(article.id)).to eq(expected_result) end diff --git a/spec/requests/notifications_spec.rb b/spec/requests/notifications_spec.rb index 540fdcd8c..ae08b394a 100644 --- a/spec/requests/notifications_spec.rb +++ b/spec/requests/notifications_spec.rb @@ -215,7 +215,7 @@ RSpec.describe "NotificationsIndex" do end it "does group notifications that are on different days but have the same reactable" do - mock_heart_reaction_notifications(2, %w[unicorn like readinglist]) + mock_heart_reaction_notifications(2, %w[unicorn like]) Notification.last.update(created_at: Notification.last.created_at - 1.day) get "/notifications" notifications = controller.instance_variable_get(:@notifications) @@ -223,8 +223,8 @@ RSpec.describe "NotificationsIndex" do end it "does not group notifications that are on the same day but have different reactables" do - mock_heart_reaction_notifications(1, %w[unicorn like readinglist], article1) - mock_heart_reaction_notifications(1, %w[unicorn like readinglist], article2) + mock_heart_reaction_notifications(1, %w[unicorn like], article1) + mock_heart_reaction_notifications(1, %w[unicorn like], article2) get "/notifications" notifications = controller.instance_variable_get(:@notifications) expect(notifications.count).to eq 2 @@ -294,7 +294,7 @@ RSpec.describe "NotificationsIndex" do end it "does group notifications that are on different days but have the same reactable" do - mock_heart_reaction_notifications(2, %w[unicorn like readinglist]) + mock_heart_reaction_notifications(2, %w[unicorn like]) Notification.last.update(created_at: Notification.last.created_at - 1.day) get notifications_path(filter: :org, org_id: organization.id) @@ -303,8 +303,8 @@ RSpec.describe "NotificationsIndex" do end it "does not group notifications that are on the same day but have different reactables" do - mock_heart_reaction_notifications(1, %w[unicorn like readinglist], article1) - mock_heart_reaction_notifications(1, %w[unicorn like readinglist], article2) + mock_heart_reaction_notifications(1, %w[unicorn like], article1) + mock_heart_reaction_notifications(1, %w[unicorn like], article2) get notifications_path(filter: :org, org_id: organization.id) notifications = controller.instance_variable_get(:@notifications) diff --git a/spec/requests/reactions_spec.rb b/spec/requests/reactions_spec.rb index 5dc148378..ff0f8b6c2 100644 --- a/spec/requests/reactions_spec.rb +++ b/spec/requests/reactions_spec.rb @@ -29,7 +29,7 @@ RSpec.describe "Reactions" do { "category" => "readinglist", "count" => 0 }, { "category" => "unicorn", "count" => 0 }, ] - expect(result["article_reaction_counts"]).to eq(expected_reactions_counts) + expect(result["article_reaction_counts"]).to contain_exactly(*expected_reactions_counts) expect(result["reactions"].to_json).to eq(user.reactions.where(reactable: article).to_json) end @@ -61,7 +61,7 @@ RSpec.describe "Reactions" do { "category" => "readinglist", "count" => 0 }, { "category" => "unicorn", "count" => 0 }, ] - expect(result["article_reaction_counts"]).to eq(expected_reactions) + expect(result["article_reaction_counts"]).to contain_exactly(*expected_reactions) expect(result["reactions"]).to be_empty end