Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lightbox: Added Button to jump to current image within the conversation #7098

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions _locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -7691,5 +7691,9 @@
"icu:WhatsNew__v7.35--0": {
"messageformat": "The new filter icon next to the search box makes it easy to quickly find unread chats, but feel free to take your time deciding whether or not to leave them on read after seeing what they had to say.",
"description": "Release notes for version 7.35"
},
"icu:jumpto": {
"messageformat": "Jump to",
"description": "Jumps to the medias position in the conversation"
}
}
9 changes: 9 additions & 0 deletions stylesheets/components/Lightbox.scss
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,15 @@
}
}

&--jump {
&::before {
@include mixins.color-svg(
'../images/icons/v3/chat/chat.svg',
variables.$color-gray-15
);
}
}

&--close {
&::before {
@include mixins.color-svg(
Expand Down
27 changes: 26 additions & 1 deletion ts/components/Lightbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
ConversationType,
SaveAttachmentActionCreatorType,
} from '../state/ducks/conversations';
import { useConversationsActions } from '../state/ducks/conversations';
import type { LocalizerType } from '../types/Util';
import type { MediaItemType, MediaItemMessageType } from '../types/MediaItem';
import * as GoogleChrome from '../util/GoogleChrome';
Expand Down Expand Up @@ -94,6 +95,7 @@ export function Lightbox({
hasNextMessage,
hasPrevMessage,
}: PropsType): JSX.Element | null {
const { scrollToMessage } = useConversationsActions();
const hasThumbnails = media.length > 1;
const messageId = media.at(0)?.message.id;
const prevMessageId = usePrevious(messageId, messageId);
Expand Down Expand Up @@ -186,6 +188,21 @@ export function Lightbox({
[isViewOnce, media, saveAttachment, selectedIndex]
);

const handleJumpToConversation = useCallback(
(
_event: KeyboardEvent | React.MouseEvent<HTMLButtonElement, MouseEvent>
) => {
if (isViewOnce) {
return;
}
const mediaItem = media[selectedIndex];
const { message } = mediaItem;
const { conversationId, id } = message;
scrollToMessage(conversationId, id);
},
[isViewOnce, media, selectedIndex, scrollToMessage]
);

const handleForward = (
event: React.MouseEvent<HTMLButtonElement, MouseEvent>
) => {
Expand Down Expand Up @@ -677,6 +694,14 @@ export function Lightbox({
<div />
)}
<div className="Lightbox__controls">
{!isViewOnce ? (
<button
aria-label={i18n('icu:jumpto')}
className="Lightbox__button Lightbox__button--jump"
onClick={handleJumpToConversation}
type="button"
/>
) : null}
{!isViewOnce ? (
<button
aria-label={i18n('icu:forwardMessage')}
Expand Down Expand Up @@ -803,7 +828,7 @@ function LightboxHeader({
i18n: LocalizerType;
message: ReadonlyDeep<MediaItemMessageType>;
}): JSX.Element {
const conversation = getConversation(message.conversationId);
const conversation = getConversation(message.authorId);

const now = Date.now();

Expand Down
1 change: 1 addition & 0 deletions ts/model-types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ export type MessageAttributesType = {
verified?: boolean;
verifiedChanged?: string;

authorId: string;
id: string;
type: MessageType;
body?: string;
Expand Down
25 changes: 18 additions & 7 deletions ts/state/ducks/lightbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
import { showStickerPackPreview } from './globalModals';
import { useBoundActions } from '../../hooks/useBoundActions';
import { DataReader } from '../../sql/Client';
import { MessageModel } from '../../models/messages';

// eslint-disable-next-line local-rules/type-alias-readonlydeep
export type LightboxStateType =
Expand Down Expand Up @@ -196,6 +197,8 @@ function showLightboxForViewOnceMedia(

const { contentType } = tempAttachment;

const authorId = getAuthorId(message);

const media = [
{
attachment: tempAttachment,
Expand All @@ -208,6 +211,7 @@ function showLightboxForViewOnceMedia(
attachments: message.get('attachments') || [],
id: message.get('id'),
conversationId: message.get('conversationId'),
authorId,
receivedAt: message.get('received_at'),
receivedAtMs: Number(message.get('received_at_ms')),
sentAt: message.get('sent_at'),
Expand All @@ -228,6 +232,16 @@ function showLightboxForViewOnceMedia(
};
}

function getAuthorId(message: MessageModel) {
return (
window.ConversationController.lookupOrCreate({
serviceId: message.get('sourceServiceId'),
e164: message.get('source'),
reason: 'conversation_view.showLightBox',
})?.id || message.get('conversationId')
);
}

function filterValidAttachments(
attributes: ReadonlyMessageAttributesType
): Array<AttachmentType> {
Expand Down Expand Up @@ -278,12 +292,8 @@ function showLightbox(opts: {
const attachments = filterValidAttachments(message.attributes);
const loop = isGIF(attachments);

const authorId =
window.ConversationController.lookupOrCreate({
serviceId: message.get('sourceServiceId'),
e164: message.get('source'),
reason: 'conversation_view.showLightBox',
})?.id || message.get('conversationId');
const authorId = getAuthorId(message);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the problem here that the conversationId gets set as the authorId, the conversation id only gets set as fallback when "window.ConversationController.lookupOrCreate" is null/undefined.

But for this functionality I always need the conversationId, so I introduced a new variable "authorId" and replaced all previous usages with that one.

Is it intended that the variable got set like this in the first place and is there a gotcha that I haven't seen?

const conversationId = message.get('conversationId');
const receivedAt = message.get('received_at');
const sentAt = message.get('sent_at');

Expand All @@ -296,7 +306,8 @@ function showLightbox(opts: {
message: {
attachments: message.get('attachments') || [],
id: messageId,
conversationId: authorId,
conversationId,
authorId,
receivedAt,
receivedAtMs: Number(message.get('received_at_ms')),
sentAt,
Expand Down
2 changes: 1 addition & 1 deletion ts/types/MediaItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { MIMEType } from './MIME';

export type MediaItemMessageType = Pick<
ReadonlyMessageAttributesType,
'attachments' | 'conversationId' | 'id'
'attachments' | 'conversationId' | 'authorId' | 'id'
> & {
receivedAt: number;
receivedAtMs?: number;
Expand Down