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

Conversation

findus
Copy link

@findus findus commented Nov 21, 2024

First time contributor checklist:

Contributor checklist:

  • My contribution is not related to translations.

  • My commits are in nice logical chunks with good commit messages

  • My changes are rebased on the latest main branch

  • A npm run ready run passes successfully (more about tests here)
    (Linting errros but in node_modules)

  • My changes are ready to be shipped to users

Description

CleanShot 2024-11-21 at 14 41 08@2x

Adds a "jump to conversation" button to the Media-Lightbox to jump to the position in the conversation where the image was used.

@findus findus force-pushed the jump-to-image branch 3 times, most recently from 47974c5 to 98b0a13 Compare November 21, 2024 13:49
Lightbox:
  moved conversationId references to new authorId reference
  save authorId and conversationId separately
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?

@findus findus marked this pull request as ready for review November 22, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants