Allow duplicates in reaction palette

2021-09-13 12:04:45 -05:00 · 2021-09-13 12:04:45 -05:00 · 240585ef94
commit 240585ef94
parent 5a57e2b704
13 changed files with 160 additions and 163 deletions
--- a/stylesheets/components/CustomizingPreferredReactionsModal.scss
+++ b/stylesheets/components/CustomizingPreferredReactionsModal.scss
@ -2,7 +2,7 @@
 // SPDX-License-Identifier: AGPL-3.0-only

 .module-CustomizingPreferredReactionsModal {
-  &__reaction-picker-wrapper {
+  &__small-emoji-picker-wrapper {
    @include font-subtitle;
    align-items: center;
    display: flex;
@ -19,7 +19,7 @@
      color: $color-gray-25;
    }

-    .module-ReactionPicker {
+    .module-ReactionPickerPicker {
      margin-bottom: 2rem;
    }
  }
--- a/stylesheets/components/ReactionPickerPicker.scss
+++ b/stylesheets/components/ReactionPickerPicker.scss
@ -1,7 +1,7 @@
 // Copyright 2021 Signal Messenger, LLC
 // SPDX-License-Identifier: AGPL-3.0-only

-.module-ReactionPicker {
+.module-ReactionPickerPicker {
  $button-size: 40px;
  $button-content-size: 28px;
  $max-expected-buttons: 7;
@ -24,7 +24,7 @@

  @media (prefers-reduced-motion: no-preference) {
    animation: {
-      name: module-ReactionPicker__appear;
+      name: module-ReactionPickerPicker__appear;
      duration: 400ms;
      timing-function: $ease-out-expo;
      fill-mode: forwards;
@ -132,7 +132,7 @@
        opacity: 0;

        animation: {
-          name: module-ReactionPicker__button-appear;
+          name: module-ReactionPickerPicker__button-appear;
          duration: 400ms;
          timing-function: $ease-out-expo;
          fill-mode: forwards;
@ -213,7 +213,7 @@
        }

        @media (prefers-reduced-motion: no-preference) {
-          animation: module-ReactionPicker__button-selected 1s ease-in-out
+          animation: module-ReactionPickerPicker__button-selected 1s ease-in-out
            infinite alternate;
        }
      }
@ -221,7 +221,7 @@
  }
 }

-@keyframes module-ReactionPicker__appear {
+@keyframes module-ReactionPickerPicker__appear {
  from {
    opacity: 0;
  }
@ -231,7 +231,7 @@
  }
 }

-@keyframes module-ReactionPicker__button-appear {
+@keyframes module-ReactionPickerPicker__button-appear {
  from {
    transform: translate3d(0, 24px, 0);
    opacity: 0;
@ -243,7 +243,7 @@
  }
 }

-@keyframes module-ReactionPicker__button-selected {
+@keyframes module-ReactionPickerPicker__button-selected {
  from {
    transform: rotate(-8deg);
  }
--- a/stylesheets/manifest.scss
+++ b/stylesheets/manifest.scss
@ -70,7 +70,7 @@
@import './components/Modal.scss';
@import './components/Preferences.scss';
@import './components/ProfileEditor.scss';
-@import './components/ReactionPicker.scss';
+@import './components/ReactionPickerPicker.scss';
@import './components/SafetyNumberChangeDialog.scss';
@import './components/SafetyNumberViewer.scss';
@import './components/SearchInput.scss';
--- a/ts/background.ts
+++ b/ts/background.ts
@ -1267,9 +1267,7 @@ export async function startApp(): Promise<void> {
          return;
        }

-        const reactionPicker = document.querySelector(
-          '.module-reaction-picker'
-        );
+        const reactionPicker = document.querySelector('.module-ReactionPicker');
        if (reactionPicker) {
          return;
        }
--- a/ts/components/CustomizingPreferredReactionsModal.tsx
+++ b/ts/components/CustomizingPreferredReactionsModal.tsx
@ -9,9 +9,10 @@ import type { LocalizerType } from '../types/Util';
 import { Modal } from './Modal';
 import { Button, ButtonVariant } from './Button';
 import {
-  ReactionPicker,
-  ReactionPickerSelectionStyle,
-} from './conversation/ReactionPicker';
+  ReactionPickerPicker,
+  ReactionPickerPickerEmojiButton,
+  ReactionPickerPickerStyle,
+} from './ReactionPickerPicker';
 import { EmojiPicker } from './emoji/EmojiPicker';
 import { DEFAULT_PREFERRED_REACTION_EMOJI_SHORT_NAMES } from '../reactions/constants';
 import { convertShortName } from './emoji/lib';
@ -96,19 +97,6 @@ export function CustomizingPreferredReactionsModal({
    };
  }, [isSomethingSelected, popperElement, deselectDraftEmoji]);

-  const selected =
-    typeof selectedDraftEmojiIndex === 'number'
-      ? draftPreferredReactions[selectedDraftEmojiIndex]
-      : undefined;
-
-  const onPick = isSaving
-    ? noop
-    : (pickedEmoji: string) => {
-        selectDraftEmojiToBeReplaced(
-          draftPreferredReactions.findIndex(emoji => emoji === pickedEmoji)
-        );
-      };
-
  const hasChanged = !isEqual(
    originalPreferredReactions,
    draftPreferredReactions
@ -132,25 +120,32 @@ export function CustomizingPreferredReactionsModal({
      }}
      title={i18n('CustomizingPreferredReactions__title')}
    >
-      <div className="module-CustomizingPreferredReactionsModal__reaction-picker-wrapper">
-        <ReactionPicker
-          hasMoreButton={false}
-          i18n={i18n}
-          onPick={onPick}
-          onSetSkinTone={shouldNotBeCalled}
+      <div className="module-CustomizingPreferredReactionsModal__small-emoji-picker-wrapper">
+        <ReactionPickerPicker
+          isSomethingSelected={isSomethingSelected}
+          pickerStyle={ReactionPickerPickerStyle.Menu}
          ref={setReferenceElement}
-          preferredReactionEmoji={draftPreferredReactions}
-          selected={selected}
-          selectionStyle={ReactionPickerSelectionStyle.Menu}
-          renderEmojiPicker={shouldNotBeCalled}
-        />
+        >
+          {draftPreferredReactions.map((emoji, index) => (
+            <ReactionPickerPickerEmojiButton
+              emoji={emoji}
+              // The index is the only thing that uniquely identifies the emoji, because
+              //   there can be duplicates in the list.
+              // eslint-disable-next-line react/no-array-index-key
+              key={index}
+              onClick={() => {
+                selectDraftEmojiToBeReplaced(index);
+              }}
+              isSelected={index === selectedDraftEmojiIndex}
+            />
+          ))}
+        </ReactionPickerPicker>
        {hadSaveError
          ? i18n('CustomizingPreferredReactions__had-save-error')
          : i18n('CustomizingPreferredReactions__subtitle')}
      </div>
      {isSomethingSelected && (
        <div
-          className="module-CustomizingPreferredReactionsModal__emoji-picker-wrapper"
          ref={setPopperElement}
          style={emojiPickerPopper.styles.popper}
          {...emojiPickerPopper.attributes.popper}
@ -195,7 +190,3 @@ export function CustomizingPreferredReactionsModal({
    </Modal>
  );
 }
-
-function shouldNotBeCalled(): React.ReactElement {
-  throw new Error('This should not be called');
-}
--- a/ts/components/ReactionPickerPicker.tsx
+++ b/ts/components/ReactionPickerPicker.tsx
@ -0,0 +1,91 @@
+// Copyright 2020-2021 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import React, { CSSProperties, ReactNode, forwardRef } from 'react';
+import classNames from 'classnames';
+
+import { Emoji } from './emoji/Emoji';
+import type { LocalizerType } from '../types/Util';
+
+export enum ReactionPickerPickerStyle {
+  Picker,
+  Menu,
+}
+
+export const ReactionPickerPickerEmojiButton = React.forwardRef<
+  HTMLButtonElement,
+  {
+    emoji: string;
+    isSelected: boolean;
+    onClick: () => unknown;
+    title?: string;
+  }
+>(({ emoji, onClick, isSelected, title }, ref) => (
+  <button
+    type="button"
+    ref={ref}
+    tabIndex={0}
+    className={classNames(
+      'module-ReactionPickerPicker__button',
+      'module-ReactionPickerPicker__button--emoji',
+      isSelected && 'module-ReactionPickerPicker__button--selected'
+    )}
+    onClick={event => {
+      event.stopPropagation();
+      onClick();
+    }}
+  >
+    <Emoji size={48} emoji={emoji} title={title} />
+  </button>
+));
+
+export const ReactionPickerPickerMoreButton = ({
+  i18n,
+  onClick,
+}: Readonly<{
+  i18n: LocalizerType;
+  onClick: () => unknown;
+}>): JSX.Element => (
+  <button
+    aria-label={i18n('Reactions--more')}
+    className="module-ReactionPickerPicker__button module-ReactionPickerPicker__button--more"
+    onClick={event => {
+      event.stopPropagation();
+      onClick();
+    }}
+    tabIndex={0}
+    title={i18n('Reactions--more')}
+    type="button"
+  >
+    <div className="module-ReactionPickerPicker__button--more__dot" />
+    <div className="module-ReactionPickerPicker__button--more__dot" />
+    <div className="module-ReactionPickerPicker__button--more__dot" />
+  </button>
+);
+
+export const ReactionPickerPicker = forwardRef<
+  HTMLDivElement,
+  {
+    children: ReactNode;
+    isSomethingSelected: boolean;
+    pickerStyle: ReactionPickerPickerStyle;
+    style?: CSSProperties;
+  }
+>(({ children, isSomethingSelected, pickerStyle, style }, ref) => (
+  <div
+    className={classNames(
+      'module-ReactionPickerPicker',
+      isSomethingSelected && 'module-ReactionPickerPicker--something-selected',
+      {
+        'module-ReactionPickerPicker--picker-style':
+          pickerStyle === ReactionPickerPickerStyle.Picker,
+        'module-ReactionPickerPicker--menu-style':
+          pickerStyle === ReactionPickerPickerStyle.Menu,
+      }
+    )}
+    ref={ref}
+    style={style}
+  >
+    {children}
+  </div>
+));
--- a/ts/components/conversation/ReactionPicker.stories.tsx
+++ b/ts/components/conversation/ReactionPicker.stories.tsx
@ -8,11 +8,7 @@ import { action } from '@storybook/addon-actions';

 import { setup as setupI18n } from '../../../js/modules/i18n';
 import enMessages from '../../../_locales/en/messages.json';
-import {
-  Props as ReactionPickerProps,
-  ReactionPicker,
-  ReactionPickerSelectionStyle,
-} from './ReactionPicker';
+import { Props as ReactionPickerProps, ReactionPicker } from './ReactionPicker';
 import { EmojiPicker } from '../emoji/EmojiPicker';

 const i18n = setupI18n('en', enMessages);
@ -47,7 +43,6 @@ storiesOf('Components/Conversation/ReactionPicker', module)
        )}
        preferredReactionEmoji={preferredReactionEmoji}
        renderEmojiPicker={renderEmojiPicker}
-        selectionStyle={ReactionPickerSelectionStyle.Picker}
      />
    );
  })
@ -64,7 +59,6 @@ storiesOf('Components/Conversation/ReactionPicker', module)
          )}
          preferredReactionEmoji={preferredReactionEmoji}
          renderEmojiPicker={renderEmojiPicker}
-          selectionStyle={ReactionPickerSelectionStyle.Picker}
        />
      </div>
    ));
--- a/ts/components/conversation/ReactionPicker.tsx
+++ b/ts/components/conversation/ReactionPicker.tsx
@ -2,20 +2,17 @@
 // SPDX-License-Identifier: AGPL-3.0-only

 import * as React from 'react';
-import classNames from 'classnames';
-import * as log from '../../logging/log';
-import { Emoji } from '../emoji/Emoji';
 import { convertShortName } from '../emoji/lib';
 import { Props as EmojiPickerProps } from '../emoji/EmojiPicker';
-import { missingCaseError } from '../../util/missingCaseError';
 import { useRestoreFocus } from '../../util/hooks/useRestoreFocus';
 import { LocalizerType } from '../../types/Util';
 import { canCustomizePreferredReactions } from '../../util/canCustomizePreferredReactions';
-
-export enum ReactionPickerSelectionStyle {
-  Picker,
-  Menu,
-}
+import {
+  ReactionPickerPicker,
+  ReactionPickerPickerEmojiButton,
+  ReactionPickerPickerMoreButton,
+  ReactionPickerPickerStyle,
+} from '../ReactionPickerPicker';

 export type RenderEmojiPickerProps = Pick<Props, 'onClose' | 'style'> &
  Pick<
@ -26,10 +23,8 @@ export type RenderEmojiPickerProps = Pick<Props, 'onClose' | 'style'> &
  };

 export type OwnProps = {
-  hasMoreButton?: boolean;
  i18n: LocalizerType;
  selected?: string;
-  selectionStyle: ReactionPickerSelectionStyle;
  onClose?: () => unknown;
  onPick: (emoji: string) => unknown;
  onSetSkinTone: (tone: number) => unknown;
@ -40,38 +35,9 @@ export type OwnProps = {

 export type Props = OwnProps & Pick<React.HTMLProps<HTMLDivElement>, 'style'>;

-const EmojiButton = React.forwardRef<
-  HTMLButtonElement,
-  {
-    emoji: string;
-    onSelect: () => unknown;
-    selected: boolean;
-    title?: string;
-  }
->(({ emoji, onSelect, selected, title }, ref) => (
-  <button
-    type="button"
-    key={emoji}
-    ref={ref}
-    tabIndex={0}
-    className={classNames(
-      'module-ReactionPicker__button',
-      'module-ReactionPicker__button--emoji',
-      selected && 'module-ReactionPicker__button--selected'
-    )}
-    onClick={e => {
-      e.stopPropagation();
-      onSelect();
-    }}
-  >
-    <Emoji size={48} emoji={emoji} title={title} />
-  </button>
-));
-
 export const ReactionPicker = React.forwardRef<HTMLDivElement, Props>(
  (
    {
-      hasMoreButton = true,
      i18n,
      onClose,
      onPick,
@ -80,7 +46,6 @@ export const ReactionPicker = React.forwardRef<HTMLDivElement, Props>(
      preferredReactionEmoji,
      renderEmojiPicker,
      selected,
-      selectionStyle,
      style,
    },
    ref
@ -130,80 +95,63 @@ export const ReactionPicker = React.forwardRef<HTMLDivElement, Props>(
      selected && !preferredReactionEmoji.includes(selected);

    let moreButton: React.ReactNode;
-    if (!hasMoreButton) {
-      moreButton = undefined;
-    } else if (otherSelected) {
+    if (otherSelected) {
      moreButton = (
-        <EmojiButton
+        <ReactionPickerPickerEmojiButton
          emoji={selected}
-          onSelect={() => {
+          onClick={() => {
            onPick(selected);
          }}
-          selected
+          isSelected
          title={i18n('Reactions--remove')}
        />
      );
    } else {
      moreButton = (
-        <button
-          aria-label={i18n('Reactions--more')}
-          className="module-ReactionPicker__button module-ReactionPicker__button--more"
-          onClick={event => {
-            event.stopPropagation();
+        <ReactionPickerPickerMoreButton
+          i18n={i18n}
+          onClick={() => {
            setPickingOther(true);
          }}
-          tabIndex={0}
-          title={i18n('Reactions--more')}
-          type="button"
-        >
-          <div className="module-ReactionPicker__button--more__dot" />
-          <div className="module-ReactionPicker__button--more__dot" />
-          <div className="module-ReactionPicker__button--more__dot" />
-        </button>
+        />
      );
    }

-    let selectionStyleClassName: string;
-    switch (selectionStyle) {
-      case ReactionPickerSelectionStyle.Picker:
-        selectionStyleClassName = 'module-ReactionPicker--picker-style';
-        break;
-      case ReactionPickerSelectionStyle.Menu:
-        selectionStyleClassName = 'module-ReactionPicker--menu-style';
-        break;
-      default:
-        log.error(missingCaseError(selectionStyle));
-        selectionStyleClassName = 'module-ReactionPicker--picker-style';
-        break;
-    }
+    // This logic is here to avoid selecting duplicate emoji.
+    let hasSelectedSomething = false;

    return (
-      <div
+      <ReactionPickerPicker
+        isSomethingSelected={typeof selected === 'number'}
+        pickerStyle={ReactionPickerPickerStyle.Picker}
        ref={ref}
        style={style}
-        className={classNames(
-          'module-ReactionPicker',
-          selectionStyleClassName,
-          selected ? 'module-ReactionPicker--something-selected' : undefined
-        )}
      >
        {preferredReactionEmoji.map((emoji, index) => {
          const maybeFocusRef = index === 0 ? focusRef : undefined;

+          const isSelected = !hasSelectedSomething && emoji === selected;
+          if (isSelected) {
+            hasSelectedSomething = true;
+          }
+
          return (
-            <EmojiButton
+            <ReactionPickerPickerEmojiButton
              emoji={emoji}
-              key={emoji}
-              onSelect={() => {
+              isSelected={isSelected}
+              // The index is the only thing that uniquely identifies the emoji, because
+              //   there can be duplicates in the list.
+              // eslint-disable-next-line react/no-array-index-key
+              key={index}
+              onClick={() => {
                onPick(emoji);
              }}
              ref={maybeFocusRef}
-              selected={emoji === selected}
            />
          );
        })}
        {moreButton}
-      </div>
+      </ReactionPickerPicker>
    );
  }
 );
--- a/ts/reactions/getPreferredReactionEmoji.ts
+++ b/ts/reactions/getPreferredReactionEmoji.ts
@ -15,15 +15,10 @@ export function getPreferredReactionEmoji(
  const isStoredValueValid =
    Array.isArray(storedValue) &&
    storedValue.length === PREFERRED_REACTION_EMOJI_COUNT &&
-    storedValue.every(isValidReactionEmoji) &&
-    !hasDuplicates(storedValue);
+    storedValue.every(isValidReactionEmoji);
  return isStoredValueValid
    ? storedValue
    : DEFAULT_PREFERRED_REACTION_EMOJI_SHORT_NAMES.map(shortName =>
        convertShortName(shortName, skinTone)
      );
 }
-
-function hasDuplicates(arr: ReadonlyArray<unknown>): boolean {
-  return new Set(arr).size !== arr.length;
-}
--- a/ts/state/ducks/preferredReactions.ts
+++ b/ts/state/ducks/preferredReactions.ts
@ -248,10 +248,7 @@ export function reducer(
        draftPreferredReactions,
        selectedDraftEmojiIndex,
      } = customizePreferredReactionsModal;
-      if (
-        selectedDraftEmojiIndex === undefined ||
-        draftPreferredReactions.includes(newEmoji)
-      ) {
+      if (selectedDraftEmojiIndex === undefined) {
        return state;
      }

--- a/ts/state/smart/ReactionPicker.tsx
+++ b/ts/state/smart/ReactionPicker.tsx
@ -13,7 +13,6 @@ import { getPreferredReactionEmoji } from '../selectors/items';
 import { LocalizerType } from '../../types/Util';
 import {
  ReactionPicker,
-  ReactionPickerSelectionStyle,
  Props,
 } from '../../components/conversation/ReactionPicker';

@ -51,7 +50,6 @@ export const SmartReactionPicker = React.forwardRef<
      }
      preferredReactionEmoji={preferredReactionEmoji}
      ref={ref}
-      selectionStyle={ReactionPickerSelectionStyle.Picker}
      {...props}
    />
  );
--- a/ts/test-both/reactions/getPreferredReactionEmoji_test.ts
+++ b/ts/test-both/reactions/getPreferredReactionEmoji_test.ts
@ -24,8 +24,6 @@ describe('getPreferredReactionEmoji', () => {
      ['❤️', '👍', 'x', '😂', '😮', '😢'],
      ['❤️', '👍', 'garbage!!', '😂', '😮', '😢'],
      ['❤️', '👍', '✨✨', '😂', '😮', '😢'],
-      // Has duplicates
-      ['❤️', '👍', '👍', '😂', '😮', '😢'],
    ].forEach(input => {
      assert.deepStrictEqual(getPreferredReactionEmoji(input, 2), [
        '❤️',
--- a/ts/test-both/state/ducks/preferredReactions_test.ts
+++ b/ts/test-both/state/ducks/preferredReactions_test.ts
@ -180,19 +180,6 @@ describe('preferred reactions duck', () => {
      assert.strictEqual(result, stateWithOpenCustomizationModal);
    });

-    it('is a no-op if the new emoji is already in the list', () => {
-      const action = replaceSelectedDraftEmoji('✨');
-      const result = reducer(
-        stateWithOpenCustomizationModalAndSelectedEmoji,
-        action
-      );
-
-      assert.strictEqual(
-        result,
-        stateWithOpenCustomizationModalAndSelectedEmoji
-      );
-    });
-
    it('replaces the selected draft emoji and deselects', () => {
      const action = replaceSelectedDraftEmoji('🐱');
      const result = reducer(