Modernize DeliveryIssueDialog, fix outline clipping in Modal

signalapp · Aug 2, 2021 · bcb9d2d · bcb9d2d
1 parent 21ffb7c
commit bcb9d2d
Show file tree

Hide file tree

Showing 16 changed files with 154 additions and 127 deletions.
diff --git a/stylesheets/_modules.scss b/stylesheets/_modules.scss
@@ -10091,17 +10091,6 @@ $contact-modal-padding: 18px;
 
 // Module: Delivery Issue Dialog
 
-.module-delivery-issue-dialog {
-  // margin-left: auto;
-  // margin-right: auto;
-
-  @include light-theme {
-    background-color: $color-white;
-  }
-  @include dark-theme {
-    background-color: $color-gray-95;
-  }
-}
 .module-delivery-issue-dialog__image {
   text-align: center;
 }
@@ -10110,31 +10099,6 @@ $contact-modal-padding: 18px;
   margin-top: 10px;
   margin-bottom: 3px;
 }
-.module-delivery-issue-dialog__buttons {
-  text-align: right;
-  margin-top: 20px;
-  padding: 3px;
-}
-.module-delivery-issue-dialog__button {
-}
-
-.module-delivery-issue-dialog__learn-more-button {
-  @include button-reset;
-  @include button-secondary;
-  @include font-body-1-bold;
-
-  border-radius: 4px;
-  padding: 7px 14px;
-}
-.module-delivery-issue-dialog__close-button {
-  @include button-reset;
-  @include button-primary;
-  @include font-body-1-bold;
-
-  border-radius: 4px;
-  padding: 7px 14px;
-  margin-left: 12px;
-}
 
 /* Third-party module: react-contextmenu*/
 

diff --git a/stylesheets/components/Modal.scss b/stylesheets/components/Modal.scss
@@ -78,13 +78,14 @@
   &__body {
     @include font-body-1;
     margin: 0;
-    overflow: auto;
   }
 
   &--has-header {
     .module-Modal__body {
       padding: 0 16px 16px 16px;
       border-top: 1px solid transparent;
+      // If there's a header, just the body scrolls
+      overflow: auto;
 
       &--scrolled {
         @include light-theme {
@@ -100,6 +101,8 @@
 
   &--no-header {
     padding: 16px;
+    // If there's no header, the whole thing scrolls
+    overflow: auto;
   }
 
   &__button-footer {

diff --git a/ts/components/AvatarPopup.tsx b/ts/components/AvatarPopup.tsx
@@ -5,7 +5,7 @@ import * as React from 'react';
 import classNames from 'classnames';
 
 import { Avatar, Props as AvatarProps } from './Avatar';
-import { useRestoreFocus } from '../util/hooks';
+import { useRestoreFocus } from '../util/hooks/useRestoreFocus';
 
 import { LocalizerType } from '../types/Util';
 

diff --git a/ts/components/Modal.stories.tsx b/ts/components/Modal.stories.tsx
@@ -32,6 +32,18 @@ story.add('Bare bones, long', () => (
   </Modal>
 ));
 
+story.add('Bare bones, long, with button', () => (
+  <Modal i18n={i18n}>
+    <p>{LOREM_IPSUM}</p>
+    <p>{LOREM_IPSUM}</p>
+    <p>{LOREM_IPSUM}</p>
+    <p>{LOREM_IPSUM}</p>
+    <Modal.ButtonFooter>
+      <Button onClick={noop}>Okay</Button>
+    </Modal.ButtonFooter>
+  </Modal>
+));
+
 story.add('Title, X button, body, and button footer', () => (
   <Modal i18n={i18n} title="Hello world" onClose={onClose} hasXButton>
     {LOREM_IPSUM}
@@ -69,6 +81,18 @@ story.add('Long body with title', () => (
   </Modal>
 ));
 
+story.add('Long body with title and button', () => (
+  <Modal i18n={i18n} title="Hello world" onClose={onClose}>
+    <p>{LOREM_IPSUM}</p>
+    <p>{LOREM_IPSUM}</p>
+    <p>{LOREM_IPSUM}</p>
+    <p>{LOREM_IPSUM}</p>
+    <Modal.ButtonFooter>
+      <Button onClick={noop}>Okay</Button>
+    </Modal.ButtonFooter>
+  </Modal>
+));
+
 story.add('Long body with long title and X button', () => (
   <Modal
     i18n={i18n}

diff --git a/ts/components/ShortcutGuide.tsx b/ts/components/ShortcutGuide.tsx
@@ -3,7 +3,7 @@
 
 import * as React from 'react';
 import classNames from 'classnames';
-import { useRestoreFocus } from '../util/hooks';
+import { useRestoreFocus } from '../util/hooks/useRestoreFocus';
 import { LocalizerType } from '../types/Util';
 
 export type Props = {

diff --git a/ts/components/conversation/ChatSessionRefreshedDialog.tsx b/ts/components/conversation/ChatSessionRefreshedDialog.tsx
@@ -6,7 +6,7 @@ import classNames from 'classnames';
 
 import { Modal } from '../Modal';
 
-import { useRestoreFocus } from '../../util/hooks';
+import { useRestoreFocus } from '../../util/hooks/useRestoreFocus';
 
 import { LocalizerType } from '../../types/Util';
 

diff --git a/ts/components/conversation/DeliveryIssueDialog.tsx b/ts/components/conversation/DeliveryIssueDialog.tsx
@@ -3,12 +3,13 @@
 
 import * as React from 'react';
 
+import { Button, ButtonSize, ButtonVariant } from '../Button';
 import { ConversationType } from '../../state/ducks/conversations';
 import { Modal } from '../Modal';
 import { Intl } from '../Intl';
 import { Emojify } from './Emojify';
 
-import { useRestoreFocus } from '../../util/hooks';
+import { useRestoreFocus } from '../../util/hooks/useRestoreFocus';
 
 import { LocalizerType } from '../../types/Util';
 
@@ -32,7 +33,7 @@ export function DeliveryIssueDialog(props: PropsType): React.ReactElement {
 
   return (
     <Modal hasXButton={false} onClose={onClose} i18n={i18n}>
-      <div className="module-delivery-issue-dialog">
+      <section>
         <div className="module-delivery-issue-dialog__image">
           <img
             src="images/delivery-issue.svg"
@@ -53,24 +54,25 @@ export function DeliveryIssueDialog(props: PropsType): React.ReactElement {
             i18n={i18n}
           />
         </div>
-        <div className="module-delivery-issue-dialog__buttons">
-          <button
-            type="button"
-            onClick={learnMoreAboutDeliveryIssue}
-            className="module-delivery-issue-dialog__learn-more-button"
-          >
-            {i18n('DeliveryIssue--learnMore')}
-          </button>
-          <button
-            type="button"
-            onClick={onClose}
-            ref={focusRef}
-            className="module-delivery-issue-dialog__close-button"
-          >
-            {i18n('Confirmation--confirm')}
-          </button>
-        </div>
-      </div>
+      </section>
+      <Modal.ButtonFooter>
+        <Button
+          onClick={learnMoreAboutDeliveryIssue}
+          size={ButtonSize.Medium}
+          variant={ButtonVariant.Secondary}
+        >
+          {i18n('DeliveryIssue--learnMore')}
+        </Button>
+        <Button
+          onClick={onClose}
+          ref={focusRef}
+          size={ButtonSize.Medium}
+          variant={ButtonVariant.Primary}
+          className="module-delivery-issue-dialog__close-button"
+        >
+          {i18n('Confirmation--confirm')}
+        </Button>
+      </Modal.ButtonFooter>
     </Modal>
   );
 }
diff --git a/ts/components/conversation/ReactionPicker.tsx b/ts/components/conversation/ReactionPicker.tsx
@@ -6,7 +6,7 @@ import classNames from 'classnames';
 import { Emoji } from '../emoji/Emoji';
 import { convertShortName } from '../emoji/lib';
 import { Props as EmojiPickerProps } from '../emoji/EmojiPicker';
-import { useRestoreFocus } from '../../util/hooks';
+import { useRestoreFocus } from '../../util/hooks/useRestoreFocus';
 import { LocalizerType } from '../../types/Util';
 
 export type RenderEmojiPickerProps = Pick<Props, 'onClose' | 'style'> &

diff --git a/ts/components/conversation/ReactionViewer.tsx b/ts/components/conversation/ReactionViewer.tsx
@@ -7,7 +7,7 @@ import classNames from 'classnames';
 import { ContactName } from './ContactName';
 import { Avatar, Props as AvatarProps } from '../Avatar';
 import { Emoji } from '../emoji/Emoji';
-import { useRestoreFocus } from '../../util/hooks';
+import { useRestoreFocus } from '../../util/hooks/useRestoreFocus';
 import { ConversationType } from '../../state/ducks/conversations';
 import { emojiToData, EmojiData } from '../emoji/lib';
 

diff --git a/ts/components/stickers/StickerPicker.tsx b/ts/components/stickers/StickerPicker.tsx
@@ -3,7 +3,7 @@
 
 import * as React from 'react';
 import classNames from 'classnames';
-import { useRestoreFocus } from '../../util/hooks';
+import { useRestoreFocus } from '../../util/hooks/useRestoreFocus';
 import { StickerPackType, StickerType } from '../../state/ducks/stickers';
 import { LocalizerType } from '../../types/Util';
 

diff --git a/ts/components/stickers/StickerPreviewModal.tsx b/ts/components/stickers/StickerPreviewModal.tsx
@@ -10,7 +10,7 @@ import { ConfirmationDialog } from '../ConfirmationDialog';
 import { LocalizerType } from '../../types/Util';
 import { StickerPackType } from '../../state/ducks/stickers';
 import { Spinner } from '../Spinner';
-import { useRestoreFocus } from '../../util/hooks';
+import { useRestoreFocus } from '../../util/hooks/useRestoreFocus';
 
 export type OwnProps = {
   readonly onClose: () => unknown;

diff --git a/ts/models/messages.ts b/ts/models/messages.ts
@@ -1060,6 +1060,11 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
     if (isIncoming(this.attributes)) {
       return this.get('source');
     }
+    if (!isOutgoing(this.attributes)) {
+      window.log.warn(
+        'Message.getSource: Called for non-incoming/non-outoing message'
+      );
+    }
 
     return this.OUR_NUMBER;
   }
@@ -1070,6 +1075,11 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
     if (isIncoming(this.attributes)) {
       return sourceDevice;
     }
+    if (!isOutgoing(this.attributes)) {
+      window.log.warn(
+        'Message.getSourceDevice: Called for non-incoming/non-outoing message'
+      );
+    }
 
     return sourceDevice || window.textsecure.storage.user.getDeviceId();
   }
@@ -1078,6 +1088,11 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
     if (isIncoming(this.attributes)) {
       return this.get('sourceUuid');
     }
+    if (!isOutgoing(this.attributes)) {
+      window.log.warn(
+        'Message.getSourceUuid: Called for non-incoming/non-outoing message'
+      );
+    }
 
     return this.OUR_UUID;
   }

diff --git a/ts/state/selectors/message.ts b/ts/state/selectors/message.ts
@@ -242,6 +242,11 @@ export function getSource(
   if (isIncoming(message)) {
     return message.source;
   }
+  if (!isOutgoing(message)) {
+    window.log.warn(
+      'message.getSource: Called for non-incoming/non-outoing message'
+    );
+  }
 
   return ourNumber;
 }
@@ -255,6 +260,11 @@ export function getSourceDevice(
   if (isIncoming(message)) {
     return sourceDevice;
   }
+  if (!isOutgoing(message)) {
+    window.log.warn(
+      'message.getSourceDevice: Called for non-incoming/non-outoing message'
+    );
+  }
 
   return sourceDevice || ourDeviceId;
 }
@@ -266,6 +276,11 @@ export function getSourceUuid(
   if (isIncoming(message)) {
     return message.sourceUuid;
   }
+  if (!isOutgoing(message)) {
+    window.log.warn(
+      'message.getSourceUuid: Called for non-incoming/non-outoing message'
+    );
+  }
 
   return ourUuid;
 }

diff --git a/ts/util/hooks.ts → ts/util/hooks/index.ts b/ts/util/hooks.ts → ts/util/hooks/index.ts
@@ -13,49 +13,6 @@ export function usePrevious<T>(initialValue: T, currentValue: T): T {
   return result;
 }
 
-type CallbackType = (toFocus: HTMLElement | null | undefined) => void;
-
-// Restore focus on teardown
-export const useRestoreFocus = (): Array<CallbackType> => {
-  const toFocusRef = React.useRef<HTMLElement | null>(null);
-  const lastFocusedRef = React.useRef<HTMLElement | null>(null);
-
-  // We need to use a callback here because refs aren't necessarily populated on first
-  //   render. For example, ModalHost makes a top-level parent div first, and then renders
-  //   into it. And the children you pass it don't have access to that root div.
-  const setFocusRef = React.useCallback(
-    (toFocus: HTMLElement | null | undefined) => {
-      if (!toFocus) {
-        return;
-      }
-
-      // We only want to do this once.
-      if (toFocusRef.current) {
-        return;
-      }
-      toFocusRef.current = toFocus;
-
-      // Remember last-focused element, focus this new target element.
-      lastFocusedRef.current = document.activeElement as HTMLElement;
-      toFocus.focus();
-    },
-    []
-  );
-
-  React.useEffect(() => {
-    return () => {
-      // On unmount, returned focus to element focused before we set the focus
-      setTimeout(() => {
-        if (lastFocusedRef.current && lastFocusedRef.current.focus) {
-          lastFocusedRef.current.focus();
-        }
-      });
-    };
-  }, []);
-
-  return [setFocusRef];
-};
-
 export const useBoundActions = <T extends ActionCreatorsMapObject>(
   actions: T
 ): T => {