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.

Sign up for GitHub

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

Jump to bottom

Don't close sticker picker if shift is pressed when sending stickers #6477

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Don't close sticker picker if shift is pressed when sending stickers #6477

wants to merge 1 commit into from

Conversation

hackerbirds
Copy link
Contributor

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

On the mobile apps, the sticker picker stays open when we send stickers, until we decide to close it. This is useful if you want to send multiple stickers quickly. But on Desktop, the interface has to be different, and currently the sticker picker automatically closes right after sending a sticker. This is nice, but if we want to send multiple stickers quickly, we need to reopen the sticker picker, find the sticker pack we want, resend sticker etc.

This PR adds the ability to prevent the sticker picker from closing when we send a sticker if the shift key is held down.

Things to mention:

  • It also affects the sticker picker in the media editor though I don't see this as an issue I feel that it should be pointed out
  • Since there was already a shortcut making use of shift I simplified the code by "merging" the shiftHeld state with the previous shortcut code to prevent having two separate events handling shift.
  • The recent stickers would update below your eyes if you send recent stickers with the tab still open, so I put them behind a state that only updates when the sticker picker is opened. I have still lots to learn with React so let me know if this is the right thing to do.

@hackerbirds hackerbirds force-pushed the pr-sticker-send branch 2 times, most recently from 361d230 to 34c28cf Compare June 15, 2023 23:13
@hackerbirds
Copy link
Contributor Author

Dunno if that's necessary but I rebased/updated my branch.

@hackerbirds hackerbirds force-pushed the pr-sticker-send branch 2 times, most recently from c3e427b to d276c29 Compare July 13, 2023 23:50
@hackerbirds
Copy link
Contributor Author

Hi, the PR is now rebased for v6.29.0.beta-1. It was also tested again to make sure it is still working properly. Lmk if there's anything that should be changed.

jamiebuilds-signal
jamiebuilds-signal reviewed Dec 6, 2023
View reviewed changes
ts/components/stickers/StickerButton.tsx Outdated
@@ -70,6 +70,8 @@ export const StickerButton = React.memo(function StickerButtonInner({
}: Props) {
const [open, internalSetOpen] = React.useState(false);

const [shiftHeld, setShiftHeld] = React.useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

Could you rework this to just check event.shiftKey on the click event instead of tracking additional state

Copy link
Member

Choose a reason for hiding this comment

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

I see your comment below about event.shiftKey, but I think you mean event.shiftKey shouldn't be used for updating setShiftHeld. If you aren't trying to track the state I don't think this is a problem?

jamiebuilds-signal
jamiebuilds-signal reviewed Dec 6, 2023
View reviewed changes
ts/components/stickers/StickerButton.tsx Outdated
@@ -103,23 +109,31 @@ export const StickerButton = React.memo(function StickerButtonInner({
installedPacks,
onClickAddPack,
popperRoot,
setRecentStickersState,
recentStickers,
setOpen,
]);

const handlePickSticker = React.useCallback(
(packId: string, stickerId: number, url: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

If you go into the StickerPicker component you can pass the event into this method

@jamiebuilds-signal
Copy link
Member

Thank you for this PR, apologies for it taking this long to get reviewed

@hackerbirds
Copy link
Contributor Author

Thank you for this PR, apologies for it taking this long to get reviewed

No worries, I know how busy y'all have been recently.

I made the following changes using your feedback, let me know if these are okay:

  • I moved the "close on sticker pick" event inside StickerPicker's onClick which is where the event.shiftKey check is done.
  • To prevent re-rendering recent stickers, I changed the recentStickers state that was inside StickerButton and used a useRef inside StickerPicker. There is a side effect to that though: if a user sends recent stickers, then switches from the recent stickers tab to another tab, and then switches back to the recent stickers tab, nothing will have updated, because now recentStickers will only update after the sticker picker is completely closed. I don't think it's worth worrying about, but it should be pointed out still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamiebuilds-signal jamiebuilds-signal jamiebuilds-signal left review comments

Assignees

@jamiebuilds-signal jamiebuilds-signal

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hackerbirds @jamiebuilds-signal @josh-signal