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

Migrate emoji skin tone list #19741

Merged
merged 4 commits into from
May 30, 2023
Merged

Migrate emoji skin tone list #19741

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
efbc890
Migrate EmojiSkinToneList to functional component
szebniok May 29, 2023
65080f1
Use null instead of -1
szebniok May 29, 2023
7ec4f49
set the highlightedIndex to null on hover out
szebniok May 30, 2023
c2f09fa
Update src/components/EmojiPicker/EmojiSkinToneList.js
szebniok May 30, 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
101 changes: 40 additions & 61 deletions src/components/EmojiPicker/EmojiSkinToneList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import _ from 'underscore';
import React, {Component} from 'react';
import React, {useState, useCallback} from 'react';
import {View, Pressable} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../../styles/styles';
Expand All @@ -20,75 +20,54 @@ const propTypes = {
...withLocalizePropTypes,
};

class EmojiSkinToneList extends Component {
constructor(props) {
super(props);
function EmojiSkinToneList(props) {
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 didn't replicate the componentDidMount/componentDidUpdate functions, as I do not see the point of updating the highlightedIndex. We are later computing the isHiglighted prop as logical "or", which already checks if the index matches the value of props.preferredSkinTone.

Copy link
Contributor

Choose a reason for hiding this comment

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

@szebniok componentDidUpdate was introduced to solve this issue. I cannot find the exact reason why componentDidMount was added but it might be required as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sobitneupane I see. I cannot reproduce this issue on my PR, but I can do it on the original code with componentDidMount commented out.

I don't know the original reasoning behind it, but the original code used the props.preferredSkinTone in two places—as an initial value of state.highlightedIndex and inside the render function. That's way it wasn't synced properly: when the skin tone was updated in another app, the props.preferredSkinTone changed and this line of code

isHighlighted={skinToneEmoji.skinTone === this.state.highlightedIndex || skinToneEmoji.skinTone === selectedEmoji.skinTone}

was higlighting two items (with the updated props.preferredSkinTone and stale state.highlightedIndex. That's way the componentDidUpdate was added to sync the changing props.preferredSkinTone.

I don't see a reason to track the value of the props.preferredSkinTone inside our state, and choose to initially set the highlightedIndex to null.

Thanks to your comment I found one minor bug introduced by my initial solution that I fixed in today's commit.

  1. Hover over one of the menu item
  2. Hover outside of it (this sets the highlightedIndex to the props.preferredSkinTone
  3. Change the skin tone in another window
  4. Observe that two items are now highlighted

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the detailed explanation @szebniok

const [highlightedIndex, setHighlightedIndex] = useState(null);
const [isSkinToneListVisible, setIsSkinToneListVisible] = useState(false);

this.updateSelectedSkinTone = this.updateSelectedSkinTone.bind(this);

this.state = {
highlightedIndex: -1,
isSkinToneListVisible: false,
};
}

componentDidMount() {
// Get the selected skinToneEmoji based on the index
const selectedEmoji = getSkinToneEmojiFromIndex(this.props.preferredSkinTone);
this.setState({highlightedIndex: selectedEmoji.skinTone});
}

componentDidUpdate(prevProps) {
// Update the highlighted skin tone only if the selected one changes
if (prevProps.preferredSkinTone === this.props.preferredSkinTone) {
return;
}

const selectedEmoji = getSkinToneEmojiFromIndex(this.props.preferredSkinTone);
this.setState({highlightedIndex: selectedEmoji.skinTone});
}
const toggleIsSkinToneListVisible = useCallback(() => {
setIsSkinToneListVisible((prev) => !prev);
}, []);

/**
* Pass the skinTone to props and hide the picker
* @param {object} skinToneEmoji
*/
updateSelectedSkinTone(skinToneEmoji) {
this.setState((prev) => ({isSkinToneListVisible: !prev.isSkinToneListVisible, highlightedIndex: skinToneEmoji.skinTone}));
this.props.updatePreferredSkinTone(skinToneEmoji.skinTone);
function updateSelectedSkinTone(skinToneEmoji) {
toggleIsSkinToneListVisible();
setHighlightedIndex(skinToneEmoji.skinTone);
props.updatePreferredSkinTone(skinToneEmoji.skinTone);
}

render() {
const selectedEmoji = getSkinToneEmojiFromIndex(this.props.preferredSkinTone);
return (
<View style={[styles.flexRow, styles.p3, styles.ph4, styles.emojiPickerContainer]}>
{!this.state.isSkinToneListVisible && (
<Pressable
onPress={() => this.setState((prev) => ({isSkinToneListVisible: !prev.isSkinToneListVisible}))}
style={[styles.flex1, styles.flexRow, styles.alignSelfCenter, styles.justifyContentStart, styles.alignItemsCenter]}
>
<View style={[styles.emojiItem, styles.justifyContentCenter]}>
<Text style={[styles.emojiText, styles.ph2, styles.textNoWrap]}>{selectedEmoji.code}</Text>
</View>
<Text style={[styles.emojiSkinToneTitle]}>{this.props.translate('emojiPicker.skinTonePickerLabel')}</Text>
</Pressable>
)}
{this.state.isSkinToneListVisible && (
<View style={[styles.flexRow, styles.flex1]}>
{_.map(Emojis.skinTones, (skinToneEmoji) => (
<EmojiPickerMenuItem
onPress={() => this.updateSelectedSkinTone(skinToneEmoji)}
onHoverIn={() => this.setState({highlightedIndex: skinToneEmoji.skinTone})}
onHoverOut={() => this.setState({highlightedIndex: selectedEmoji.skinTone})}
key={skinToneEmoji.code}
emoji={skinToneEmoji.code}
isHighlighted={skinToneEmoji.skinTone === this.state.highlightedIndex || skinToneEmoji.skinTone === selectedEmoji.skinTone}
/>
))}
const currentSkinTone = getSkinToneEmojiFromIndex(props.preferredSkinTone);
return (
<View style={[styles.flexRow, styles.p3, styles.ph4, styles.emojiPickerContainer]}>
{!isSkinToneListVisible && (
<Pressable
onPress={toggleIsSkinToneListVisible}
style={[styles.flex1, styles.flexRow, styles.alignSelfCenter, styles.justifyContentStart, styles.alignItemsCenter]}
>
<View style={[styles.emojiItem, styles.justifyContentCenter]}>
<Text style={[styles.emojiText, styles.ph2, styles.textNoWrap]}>{currentSkinTone.code}</Text>
</View>
)}
</View>
);
}
<Text style={[styles.emojiSkinToneTitle]}>{props.translate('emojiPicker.skinTonePickerLabel')}</Text>
</Pressable>
)}
{isSkinToneListVisible && (
<View style={[styles.flexRow, styles.flex1]}>
{_.map(Emojis.skinTones, (skinToneEmoji) => (
<EmojiPickerMenuItem
onPress={() => updateSelectedSkinTone(skinToneEmoji)}
onHoverIn={() => setHighlightedIndex(skinToneEmoji.skinTone)}
onHoverOut={() => setHighlightedIndex(null)}
key={skinToneEmoji.code}
emoji={skinToneEmoji.code}
isHighlighted={skinToneEmoji.skinTone === highlightedIndex || skinToneEmoji.skinTone === currentSkinTone.skinTone}
/>
))}
</View>
)}
</View>
);
}

EmojiSkinToneList.propTypes = propTypes;
szebniok marked this conversation as resolved.
Show resolved Hide resolved
Expand Down