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

Update frequently used emojis in state whenever its onyx key changes #23965

Merged
merged 10 commits into from
Aug 2, 2023
+71 −20
Merged

Update frequently used emojis in state whenever its onyx key changes #23965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
04ab9fc
Update frequently used emojis in state whenever its onyx key changes
huzaifa-99 Jul 31, 2023
cac3ed3
Fix lint
huzaifa-99 Jul 31, 2023
4e61c76
Fix lint
huzaifa-99 Jul 31, 2023
ac20ac9
Fix prop type
huzaifa-99 Jul 31, 2023
a026502
Merge branch 'main' into 23800-update-frequently-used-emojis-in-emoji…
huzaifa-99 Aug 1, 2023
08017f7
Fix lint
huzaifa-99 Aug 1, 2023
d2f71e2
Return early in componentDidUpdate() for <EmojiPopoverMenu/>
huzaifa-99 Aug 1, 2023
079e5bf
Renamed function
huzaifa-99 Aug 1, 2023
4d7800b
Correctly set header emojis in emoji picker
huzaifa-99 Aug 2, 2023
5949859
Updated comment
huzaifa-99 Aug 2, 2023
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
68 changes: 51 additions & 17 deletions src/components/EmojiPicker/EmojiPickerMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ const propTypes = {
/** Stores user's preferred skin tone */
preferredSkinTone: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

/** Stores user's frequently used emojis */
// eslint-disable-next-line react/forbid-prop-types
frequentlyUsedEmojis: PropTypes.arrayOf(PropTypes.object),

/** Props related to the dimensions of the window */
...windowDimensionsPropTypes,

Expand All @@ -42,6 +46,7 @@ const propTypes = {
const defaultProps = {
forwardedRef: () => {},
preferredSkinTone: CONST.EMOJI_DEFAULT_SKIN_TONE,
frequentlyUsedEmojis: [],
};

class EmojiPickerMenu extends Component {
Expand All @@ -54,23 +59,6 @@ class EmojiPickerMenu extends Component {
// Ref for emoji FlatList
this.emojiList = undefined;

// If we're on Windows, don't display the flag emojis (the last category),
// since Windows doesn't support them
const flagHeaderIndex = _.findIndex(emojis, (emoji) => emoji.header && emoji.code === 'flags');
this.emojis =
getOperatingSystem() === CONST.OS.WINDOWS
? EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis.slice(0, flagHeaderIndex))
: EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis);

// Get the header emojis along with the code, index and icon.
// index is the actual header index starting at the first emoji and counting each one
this.headerEmojis = EmojiUtils.getHeaderEmojis(this.emojis);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you delete this assignment entirely? I see you are calculating it inside ComponentDidUpdate but there is no assignment to this.headerEmojis there. I see it's used in CategoryShortcutBar, and because of that it's not visible now

Copy link
Contributor

Choose a reason for hiding this comment

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

after:
image

before:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, this is a mistake. Fixing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it here, @robertKozik. Sry i missed this and thanks for the spot


// This is the indices of each header's Row
// The positions are static, and are calculated as index/numColumns (8 in our case)
// This is because each row of 8 emojis counts as one index to the flatlist
this.headerRowIndices = _.map(this.headerEmojis, (headerEmoji) => Math.floor(headerEmoji.index / CONST.EMOJI_NUM_PER_ROW));

// We want consistent auto focus behavior on input between native and mWeb so we have some auto focus management code that will
// prevent auto focus when open picker for mobile device
this.shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus();
Expand All @@ -91,6 +79,11 @@ class EmojiPickerMenu extends Component {
this.currentScrollOffset = 0;
this.firstNonHeaderIndex = 0;

const {filteredEmojis, headerEmojis, headerRowIndices} = this.getEmojisAndHeaderRowIndices();
this.emojis = filteredEmojis;
this.headerEmojis = headerEmojis;
this.headerRowIndices = headerRowIndices;

this.state = {
filteredEmojis: this.emojis,
headerIndices: this.headerRowIndices,
Expand All @@ -117,6 +110,19 @@ class EmojiPickerMenu extends Component {
this.setFirstNonHeaderIndex(this.emojis);
}

componentDidUpdate(prevProps) {
if (prevProps.frequentlyUsedEmojis === this.props.frequentlyUsedEmojis) return;

const {filteredEmojis, headerEmojis, headerRowIndices} = this.getEmojisAndHeaderRowIndices();
this.emojis = filteredEmojis;
this.headerEmojis = headerEmojis;
this.headerRowIndices = headerRowIndices;
this.setState({
filteredEmojis: this.emojis,
headerIndices: this.headerRowIndices,
});
}

componentWillUnmount() {
this.cleanupEventHandlers();
}
Expand All @@ -130,6 +136,31 @@ class EmojiPickerMenu extends Component {
this.setState({selection: event.nativeEvent.selection});
}

/**
* Calculate the filtered + header emojis and header row indices
* @returns {Object}
*/
getEmojisAndHeaderRowIndices() {
// If we're on Windows, don't display the flag emojis (the last category),
// since Windows doesn't support them
const flagHeaderIndex = _.findIndex(emojis, (emoji) => emoji.header && emoji.code === 'flags');
const filteredEmojis =
getOperatingSystem() === CONST.OS.WINDOWS
? EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis.slice(0, flagHeaderIndex))
: EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis);

// Get the header emojis along with the code, index and icon.
// index is the actual header index starting at the first emoji and counting each one
const headerEmojis = EmojiUtils.getHeaderEmojis(filteredEmojis);

// This is the indices of each header's Row
// The positions are static, and are calculated as index/numColumns (8 in our case)
// This is because each row of 8 emojis counts as one index to the flatlist
const headerRowIndices = _.map(headerEmojis, (headerEmoji) => Math.floor(headerEmoji.index / CONST.EMOJI_NUM_PER_ROW));

return {filteredEmojis, headerEmojis, headerRowIndices};
}

/**
* Find and store index of the first emoji item
* @param {Array} filteredEmojis
Expand Down Expand Up @@ -553,6 +584,9 @@ export default compose(
preferredSkinTone: {
key: ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE,
},
frequentlyUsedEmojis: {
key: ONYXKEYS.FREQUENTLY_USED_EMOJIS,
},
}),
)(
React.forwardRef((props, ref) => (
Expand Down
23 changes: 20 additions & 3 deletions src/components/EmojiPicker/EmojiPickerMenu/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useState, useMemo} from 'react';
import React, {useState, useMemo, useEffect} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import PropTypes from 'prop-types';
Expand Down Expand Up @@ -27,23 +27,37 @@ const propTypes = {
/** Stores user's preferred skin tone */
preferredSkinTone: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

/** Stores user's frequently used emojis */
// eslint-disable-next-line react/forbid-prop-types
frequentlyUsedEmojis: PropTypes.arrayOf(PropTypes.object),

/** Props related to translation */
...withLocalizePropTypes,
};

const defaultProps = {
preferredSkinTone: CONST.EMOJI_DEFAULT_SKIN_TONE,
frequentlyUsedEmojis: [],
};

function EmojiPickerMenu({preferredLocale, onEmojiSelected, preferredSkinTone, translate}) {
function EmojiPickerMenu({preferredLocale, onEmojiSelected, preferredSkinTone, translate, frequentlyUsedEmojis}) {
const emojiList = useAnimatedRef();
const allEmojis = useMemo(() => EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis), []);
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain to me why we need this? I'd just like to understand.

Copy link
Contributor Author

@huzaifa-99 huzaifa-99 Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We are calculating allEmojis whenever frequentlyUsedEmojis are changing. Since we are not directly using the frequentlyUsedEmojis value in the memo hook, it gives lint error.

The EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis() function uses the frequentlyUsedEmojis value internally by subscribing to onyx in EmojiUtils.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! Thanks for the explanation.

const allEmojis = useMemo(() => EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis), [frequentlyUsedEmojis]);
const headerEmojis = useMemo(() => EmojiUtils.getHeaderEmojis(allEmojis), [allEmojis]);
const headerRowIndices = useMemo(() => _.map(headerEmojis, (headerEmoji) => Math.floor(headerEmoji.index / CONST.EMOJI_NUM_PER_ROW)), [headerEmojis]);
const [filteredEmojis, setFilteredEmojis] = useState(allEmojis);
const [headerIndices, setHeaderIndices] = useState(headerRowIndices);
const {windowWidth} = useWindowDimensions();

useEffect(() => {
setFilteredEmojis(allEmojis);
}, [allEmojis]);

useEffect(() => {
setHeaderIndices(headerRowIndices);
}, [headerRowIndices]);

const getItemLayout = (data, index) => ({length: CONST.EMOJI_PICKER_ITEM_HEIGHT, offset: CONST.EMOJI_PICKER_ITEM_HEIGHT * index, index});

/**
Expand Down Expand Up @@ -200,6 +214,9 @@ export default compose(
preferredSkinTone: {
key: ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE,
},
frequentlyUsedEmojis: {
key: ONYXKEYS.FREQUENTLY_USED_EMOJIS,
},
}),
)(
React.forwardRef((props, ref) => (
Expand Down