Skip to content

Skin tone fix #16604

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

Merged
mountiny merged 12 commits into from
Apr 3, 2023
Merged

Skin tone fix #16604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
80d7bf9
Merge branch 'main' of https://github.com/margelo/expensify-app-fork
perunt Mar 9, 2023
6068332
Merge branch 'main' of https://github.com/margelo/expensify-app-fork
perunt Mar 15, 2023
0d04bce
Merge branch 'main' of https://github.com/margelo/expensify-app-fork
perunt Mar 27, 2023
8b7cd76
Merge branch 'main' of https://github.com/Expensify/App
perunt Mar 27, 2023
040eeef
fix skin tone on replaceEmojis
perunt Mar 28, 2023
e4d6781
Merge branch 'main' of https://github.com/Expensify/App into perunt/s…
perunt Mar 28, 2023
113fcb2
fix tests
perunt Mar 28, 2023
7768680
add eslint rule 'onyx-props-must-have-default'
perunt Mar 28, 2023
d4a9741
move definition outside component to circumvent onyx bug
perunt Mar 31, 2023
d829d81
update propTypes
perunt Mar 31, 2023
71ac667
remove required from preferredSkinTone
perunt Apr 3, 2023
3073e4b
use preferredSkinTone from CONST
perunt Apr 3, 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
5 changes: 3 additions & 2 deletions src/libs/EmojiUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,10 @@ function addToFrequentlyUsedEmojis(frequentlyUsedEmojis, newEmoji) {
* If we're on mobile, we also add a space after the emoji granted there's no text after it.
* @param {String} text
* @param {Boolean} isSmallScreenWidth
* @param {Number} preferredSkinTone
* @returns {String}
*/
function replaceEmojis(text, isSmallScreenWidth = false) {
function replaceEmojis(text, isSmallScreenWidth = false, preferredSkinTone = CONST.EMOJI_DEFAULT_SKIN_TONE) {
let newText = text;
const emojiData = text.match(CONST.REGEX.EMOJI_NAME);
if (!emojiData || emojiData.length === 0) {
Expand All @@ -201,7 +202,7 @@ function replaceEmojis(text, isSmallScreenWidth = false) {
for (let i = 0; i < emojiData.length; i++) {
const checkEmoji = emojisTrie.search(emojiData[i].slice(1, -1));
if (checkEmoji && checkEmoji.metaData.code) {
let emojiReplacement = checkEmoji.metaData.code;
let emojiReplacement = this.getEmojiCodeWithSkinColor(checkEmoji.metaData, preferredSkinTone);

@fedirjh fedirjh Apr 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line caused this regression #16896 , We should remove the this. keyword and simply call the getEmojiCodeWithSkinColor() function directly.

@s77rt s77rt Apr 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How come I was not able to reproduce this on localhost? Is this caused by the diff between debug javascript files and the built ones?

@fedirjh fedirjh Apr 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this keyword refer to the module EmojiUtils, on dev environment variables and function names are preserved , When code is minified during production, function and variable names are often changed to shorter, more efficient names. This can cause issues if you are using this to access functions or entities, as the new names may not match the original names.

Dev Same module on production
Screenshot 2023-04-04 at 5 34 42 AM Screenshot 2023-04-04 at 5 34 23 AM

@Beamanator Beamanator Apr 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good find, and that's quite interesting about this. working on Dev but not production 🤔 Maybe we need some better eslint rules to catch this in the future?

@s77rt s77rt Apr 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Beamanator That would be good. Can you please create a tracking issue for this one? The lint rule is good approach to prevent this bug but what about other unknown diff between dev and built js files?


// If this is the last emoji in the message and it's the end of the message so far,
// add a space after it so the user can keep typing easily.
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ class ReportActionCompose extends React.Component {
* @param {Boolean} shouldDebounceSaveComment
*/
updateComment(comment, shouldDebounceSaveComment) {
const newComment = EmojiUtils.replaceEmojis(comment, this.props.isSmallScreenWidth);
const newComment = EmojiUtils.replaceEmojis(comment, this.props.isSmallScreenWidth, this.props.preferredSkinTone);
this.setState((prevState) => {
const newState = {
isCommentEmpty: !!newComment.match(/^(\s)*$/),
Expand Down
13 changes: 13 additions & 0 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import lodashGet from 'lodash/get';
import React, {Component} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import CONST from '../../../CONST';
import ONYXKEYS from '../../../ONYXKEYS';
import reportActionPropTypes from './reportActionPropTypes';
Expand Down Expand Up @@ -64,12 +65,16 @@ const propTypes = {
/** Draft message - if this is set the comment is in 'edit' mode */
draftMessage: PropTypes.string,

/** Stores user's preferred skin tone */
preferredSkinTone: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

...windowDimensionsPropTypes,
};

const defaultProps = {
draftMessage: '',
hasOutstandingIOU: false,
preferredSkinTone: CONST.EMOJI_DEFAULT_SKIN_TONE,
};

class ReportActionItem extends Component {
Expand Down Expand Up @@ -192,6 +197,9 @@ class ReportActionItem extends Component {
index={this.props.index}
ref={el => this.textInput = el}
report={this.props.report}

// Avoid defining within component due to an existing Onyx bug
preferredSkinTone={this.props.preferredSkinTone}
shouldDisableEmojiPicker={
(ReportUtils.chatIncludesConcierge(this.props.report) && User.isBlockedFromConcierge(this.props.blockedFromConcierge))
|| ReportUtils.isArchivedRoom(this.props.report)
Expand Down Expand Up @@ -314,4 +322,9 @@ export default compose(
return lodashGet(drafts, draftKey, '');
},
}),
withOnyx({
preferredSkinTone: {
key: ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE,
},
}),
)(ReportActionItem);
7 changes: 6 additions & 1 deletion src/pages/home/report/ReportActionItemMessageEdit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable rulesdir/onyx-props-must-have-default */
import lodashGet from 'lodash/get';
import React from 'react';
import {InteractionManager, Keyboard, View} from 'react-native';
Expand Down Expand Up @@ -53,6 +54,9 @@ const propTypes = {
/** Whether or not the emoji picker is disabled */
shouldDisableEmojiPicker: PropTypes.bool,

/** Stores user's preferred skin tone */
preferredSkinTone: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

...withLocalizePropTypes,
...windowDimensionsPropTypes,
...keyboardStatePropTypes,
Expand All @@ -63,6 +67,7 @@ const defaultProps = {
report: {},
shouldDisableEmojiPicker: false,
numberOfLines: 1,
preferredSkinTone: CONST.EMOJI_DEFAULT_SKIN_TONE,
};

class ReportActionItemMessageEdit extends React.Component {
Expand Down Expand Up @@ -121,7 +126,7 @@ class ReportActionItemMessageEdit extends React.Component {
* @param {String} draft
*/
updateDraft(draft) {
const newDraft = EmojiUtils.replaceEmojis(draft, this.props.isSmallScreenWidth);
const newDraft = EmojiUtils.replaceEmojis(draft, this.props.isSmallScreenWidth, this.props.preferredSkinTone);
Comment thread
perunt marked this conversation as resolved.
this.setState((prevState) => {
const newState = {draft: newDraft};
if (draft !== newDraft) {
Expand Down