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

fix: 24261 user able to select 2 emoji at a time #24614

Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import * as User from '../../../libs/actions/User';
import TextInput from '../../TextInput';
import CategoryShortcutBar from '../CategoryShortcutBar';
import * as StyleUtils from '../../../styles/StyleUtils';
import useSingleExecution from '../../../hooks/useSingleExecution';

const propTypes = {
/** Function to add the selected emoji to the main compose text input */
Expand Down Expand Up @@ -49,6 +50,7 @@ function EmojiPickerMenu({preferredLocale, onEmojiSelected, preferredSkinTone, t
const [filteredEmojis, setFilteredEmojis] = useState(allEmojis);
const [headerIndices, setHeaderIndices] = useState(headerRowIndices);
const {windowWidth} = useWindowDimensions();
const {singleExecution} = useSingleExecution();

useEffect(() => {
setFilteredEmojis(allEmojis);
Expand Down Expand Up @@ -150,7 +152,7 @@ function EmojiPickerMenu({preferredLocale, onEmojiSelected, preferredSkinTone, t

return (
<EmojiPickerMenuItem
onPress={(emoji) => addToFrequentAndSelectEmoji(emoji, item)}
onPress={singleExecution((emoji) => addToFrequentAndSelectEmoji(emoji, item))}
emoji={emojiCode}
/>
);
Expand Down
39 changes: 22 additions & 17 deletions src/hooks/useSingleExecution.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {InteractionManager} from 'react-native';
import {useCallback, useState} from 'react';
import {useCallback, useState, useRef} from 'react';

/**
* With any action passed in, it will only allow 1 such action to occur at a time.
Expand All @@ -8,27 +8,32 @@ import {useCallback, useState} from 'react';
*/
export default function useSingleExecution() {
const [isExecuting, setIsExecuting] = useState(false);
const isExecutingRef = useRef();

const singleExecution = useCallback(
(action) => () => {
if (isExecuting) {
return;
}

setIsExecuting(true);
isExecutingRef.current = isExecuting;

const execution = action();
InteractionManager.runAfterInteractions(() => {
if (!(execution instanceof Promise)) {
setIsExecuting(false);
const singleExecution = useCallback(
(action) =>
(...params) => {
if (isExecutingRef.current) {
return;
}
execution.finally(() => {
setIsExecuting(false);

setIsExecuting(true);
isExecutingRef.current = true;

Copy link
Contributor

Choose a reason for hiding this comment

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

@tienifr Hi, we are planning to use this hook in this issue and I'm expecting the isExecutingRef to be updated immediately on click instead of on each render. Is there any specific reason for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isExecutingRef is updated right after singleExecution is executed. Why do you think it's updated on each render?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the isExecutingRef.current is updated at line 13 which I guess will be updated when useSingleExecution function is invoked again on a re-render.

I expect it to look like this:

setIsExecuting(true);
isExecutingRef.current = true;

CMIIW

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bernhardoj Thanks for your suggestion. I totally agree with your idea. Updated the PR

const execution = action(params);
Copy link
Contributor

Choose a reason for hiding this comment

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

@tienifr Any reason why we're not spreading params here?

InteractionManager.runAfterInteractions(() => {
if (!(execution instanceof Promise)) {
setIsExecuting(false);
return;
}
execution.finally(() => {
setIsExecuting(false);
});
});
});
},
[isExecuting],
},
[],
);

return {isExecuting, singleExecution};
Expand Down