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

Save current image from the gallery view by pressing 's', 'd', or 'Enter' #6724

Merged
merged 1 commit into from Jan 4, 2024

Conversation

pelya
Copy link
Contributor

@pelya pelya commented Dec 18, 2023

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 yarn ready run passes successfully (more about tests here)
  • My changes are ready to be shipped to users

Description

This commit will allow to save images from a gallery view by pressing Enter, 's', or 'd' keys.
To test:

  • Open a conversation containing 10+ images attached to a single message.
  • Click on the first image, an image gallery will open.
  • Press 'Enter' to download the first image.
  • Press 'Right' then 'Enter' to download subsequent images.
  • This improves accessibility - no need to right-click on every image and select 'Download' from the popup menu.

@trevor-signal trevor-signal self-assigned this Dec 19, 2023
@trevor-signal
Copy link
Contributor

Hi @pelya, thanks for your contribution. I spoke with our design team and we would be happy with this change if we modified it to be "cmd + s" (on macOS, else "ctrl + s") shortcut, rather than the three keys you have now. Would you be interested in making that change?

@pelya pelya force-pushed the save-gallery-image-with-enter branch from 52ce24a to ef67a98 Compare December 19, 2023 20:07
@pelya
Copy link
Contributor Author

pelya commented Dec 19, 2023

I've changed key shortcut to Ctrl-S

@@ -211,10 +213,16 @@ export function Lightbox({
onNext(event);
break;

case 's':
if (isMacOS ? event.metaKey : event.ctrlKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mind using the isCmdOrCtrl utility function we have?

@trevor-signal trevor-signal added the PR: Needs CLA Contributor License Agreement needs to be signed! label Dec 20, 2023
@trevor-signal
Copy link
Contributor

I've changed key shortcut to Ctrl-S

Thanks! One small comment, and do you mind signing the CLA? That's required for outside contributions.

@pelya
Copy link
Contributor Author

pelya commented Dec 20, 2023 via email

@trevor-signal trevor-signal removed the PR: Needs CLA Contributor License Agreement needs to be signed! label Dec 22, 2023
@trevor-signal
Copy link
Contributor

Thanks, I've updated it and merged into our private repo. This will be released early next year! Thanks 👍

@ayumi-signal ayumi-signal merged commit ef67a98 into signalapp:main Jan 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants