Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[pay 2021-09-17] Add skin-tone support for Emojis #4323

Closed mananjadhav closed 3 years ago

mananjadhav commented 3 years ago

If you havenโ€™t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

NA

Expected Result:

Actual Result:

image

Workaround:

Yes

Platform:

Where is this issue occurring?

Version Number: Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL:

View all open jobs on Upwork

Proposed Solution

Replace the current component with an external library

https://github.com/missive/emoji-mart

MelvinBot commented 3 years ago

Triggered auto assignment to @conorpendergrast (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

conorpendergrast commented 3 years ago

Big fan in particular of supporting skin tones rather than the single current skin tone.

For the solution, I think we can do this without implementing an external library, right? We can just use the unicode modifiers. Passing to engineering to review and confirm that solution would work

MelvinBot commented 3 years ago

Triggered auto assignment to @TomatoToaster (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

mananjadhav commented 3 years ago

@conorpendergrast Sure that can be done too. We can extend the current list by storing their unicodes.

The picker UI could look something like this (with a picker for skin-tone):

image

TomatoToaster commented 3 years ago

Ah TIL how skin tones work with emoji sets like this! I think that modifier part will work ๐Ÿ‘๐Ÿพ .

I'm not sure how you'll save the modifier. I'm thinking that you might have to store it in an NVP for the user, but maybe there's another solution I'm not aware of. For the proposal, if it's concluded that there needs to be some internal work done to store that information for a user, I'm happy to help out.

MelvinBot commented 3 years ago

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

mananjadhav commented 3 years ago

@TomatoToaster Currently what we are doing is storing the rendered emoji as is.

Storing the skin tone for user as NVP should help.

Iโ€™ve raised a separate issue for typing the emoji code too. https://github.com/Expensify/App/issues/4324

Weโ€™ve got two approaches to solve this.

  1. Always store the Unicode in the text like :thumbs_up:skin-tone-2: and render to and fro from Markdown and HTML.

  2. We can render the Emoji when the user chooses from the Emoji picker based on the skin tone nvp and store the rendered emoji in the chat

stitesExpensify commented 3 years ago

2 things to mention here:

  1. WRT the original solution, the suggested library is written in react, not react native, so I don't think it would work for our purposes
  2. We already have skin tones saved for all emojis that support skin tone in the types field. My plan for skin tones was to just create a new NVP something like preferredUserSkinTone that stores the index of the skin tone they chose. Then for every emoji, if emoji.type exists, just insert emoji.type[x] instead of emoji.code. What do you think of that solution?

Additionally, (and I know you couldn't have known this @mananjadhav) we already have a mockup for what we want the emoji picker to look like including the skin tone picker :smile: 2021-08-02_10-07-59

mananjadhav commented 3 years ago

@stitesExpensify I did check the emoji array and the types that we store. My second approach is exactly what youโ€™ve mentioned, picking the emojis based on skinTone types should work.

As I also mentioned earlier would need an nvp to be stored on the backend.

Checked the mock-up. Those look great. One open issue I saw somewhere was that the current emoji picker doesnโ€™t work well in the landscape mode, hence I didnโ€™t put the picker at the bottom like slack.

stitesExpensify commented 3 years ago

Gotcha. I think we'll stick to the mockups here, and fix the picker in landscape mode if necessary.

This doesn't actually require any backend changes though. You can just create a new NVP with the name expensify_emojiSkinToneIndex from the front end :smile:

mananjadhav commented 3 years ago

@stitesExpensify I've managed to get some pieces working and send in a Proposal

Approach

  1. Add const skinTones = ['๐Ÿ–๐Ÿฟ', '๐Ÿ–๐Ÿพ', '๐Ÿ–๐Ÿฝ', '๐Ÿ–๐Ÿผ', '๐Ÿ–๐Ÿป']; to the emojis.js
  2. Maximum changes are in EmojiPickerMenu/index.js
// Add flags in the state
this.state = {
            preferredSkinTone: undefined,
            highlightedSkinToneIndex: -1,
            ... // old state vars
        };
// Add preferredSkinTone to props and defaultProps as     preferredSkinTone: PropTypes.number and preferredSkinTone: null,
// Set state from Props
componentDidMount() {
      // Other code before this
      this.state.preferredSkinTone = this.props.preferredSkinTone;
}

// Update preferred skin tone
setPreferredSkinTone(skinToneIndex) {
        this.setState({preferredSkinTone: skinToneIndex});
        if (this.props.updateSelectedSkinTone) {
            this.props.updateSelectedSkinTone(skinToneIndex); // Onyx call will come here
        }
    }

// renderItem
renderItem({item, index}) {
        const {code, header, types} = item;

         if (code === CONST.EMOJI_SPACER) {
            return null;
        }

        let emojiCode = code;
        if (types && this.state.preferredSkinTone && types[this.state.preferredSkinTone]) {
            emojiCode = types[this.state.preferredSkinTone];
        }

        return (
            <EmojiPickerMenuItem
                onPress={this.props.onEmojiSelected}
                onHover={() => this.setState({highlightedIndex: index})}
                emoji={`${code}\uFE0F`}
                emoji={`${emojiCode}\uFE0F`}
                isHighlighted={index === this.state.highlightedIndex}
                emojiSize={this.emojiSize}
            />
}

// Update FlatList to render on preferredSkinTone : wip
// Add to FlatList
 extraData={[this.state.filteredEmojis, this.state.highlightedIndex, this.state.preferredSkinTone]}

// Skin tone picker: Needs design work

 <View style={[styles.flexRow]}>
                    {
                      skinTones.map((skinTone, index) => (
                          <EmojiPickerMenuItem
                              onPress={() => this.setPreferredSkinTone(index)}
                              onHover={() => this.setState({highlightedSkinToneIndex: index})}
                              emoji={`${skinTone}\uFE0F`}
                              isHighlighted={index === this.state.highlightedSkinToneIndex}
                              emojiSize={this.emojiSize}
                          />
                      ))
                    }
                </View>
  1. Onyx related changes
    
    // Add to ONYXKEYS.js
    PREFERRED_SKIN_TONE: 'expensify_emojiSkinToneIndex',

// ReportActionCompose

withOnyx({ // other keys, preferredSkinTone: { key: ONYXKEYS.PREFERRED_SKIN_TONE } });

<EmojiPickerMenu onEmojiSelected={this.addEmojiToTextBox} preferredSkinTone={this.props.preferredSkinTone} updateSelectedSkinTone={this.updateSelectedSkinTone} ref={el => this.emojiSearchInput = el} />

updateSelectedSkinTone(skinToneIndex) { Onyx.set(ONYXKEYS.PREFERRED_SKIN_TONE, skinToneIndex); }



### Caveats:

1. Assumption is that length and the order of the skin-tones and types array are the same.
2. Need the proper design to implement the picker. I can see that the picker mockup also has category icons. I haven't factored those in this one.

Phew... Turns out a changes are a little more lines than I expected.
MelvinBot commented 3 years ago

@puneetlath Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

stitesExpensify commented 3 years ago

Hi @mananjadhav that looks good to me! @puneetlath can we make an upwork job for this and hire @mananjadhav ?

Add const skinTones = ['๐Ÿ–๐Ÿฟ', '๐Ÿ–๐Ÿพ', '๐Ÿ–๐Ÿฝ', '๐Ÿ–๐Ÿผ', '๐Ÿ–๐Ÿป']; to the emojis.js

What is the purpose of this?

Assumption is that length and the order of the skin-tones and types array are the same.

AFAICT they are. We can fix the emojis that aren't later if necessary.

Need the proper design to implement the picker. I can see that the picker mockup also has category icons. I haven't factored those in this one.

Can you just use the design I posted before, while ignoring the categories section?

mananjadhav commented 3 years ago

@stitesExpensify skinTones array is to populate the skin tone list at the bottom of the picker

stitesExpensify commented 3 years ago

Ah, totally makes sense. Thanks for the clarification

mananjadhav commented 3 years ago

While @puneetlath creates a job and sends an invite, I'll get started.

btw @stitesExpensify I am guessing the Emojipicker by itself works responsive on all devices and I don't have to worry about the container?

Apart from what we've discussed anything else you think I should be taking care of?

stitesExpensify commented 3 years ago

btw @stitesExpensify I am guessing the Emojipicker by itself works responsive on all devices and I don't have to worry about the container?

emojiPickerMenu.js has both an index and index.native so we'll need to update both.

Apart from what we've discussed anything else you think I should be taking care of?

The only thing I can think of is that your code doesn't include any server-side saving of the emoji preference. We are going to want to store that in a new NVP (by calling NameValuePair.set) called preferredUserSkinTone. We can then get that on app load and save it in onyx to be used by the emoji picker.

mananjadhav commented 3 years ago

Thanks for the note. I'll check these and test.

puneetlath commented 3 years ago

Just chatting internally about what the job size should be. Will create the Upwork job in the next day or two.

@mananjadhav do you need any design assets? Or do you have everything you need from a design perspective?

mananjadhav commented 3 years ago

@puneetlath I don't think I need any. Because the skin tones are already available in the emoji list. If there's something specific you guys already have, then do share. Being a bigger task, I'll be taking the weekend to complete this.

mananjadhav commented 3 years ago

@stitesExpensify Work started, you can do a quick review with the PR on the direction and flow. I'll close it soon.

Quick question on getting NVP.

I am calling the following code in the AuthScreen. I am a little confused it should be Report page or AuthScreen?

NameValuePair.get(CONST.NVP.PREFERRED_SKIN_TONE, ONYXKEYS.NVP_PREFERRED_SKIN_TONE, undefined);.

AuthScreen should be fine so that if any other page uses it in the future we know, can subscribe, and get updated.

mananjadhav commented 3 years ago

Quick clarification on one more item, Should I maintain the preferredSkinTone in the EmojiPickerMenu state or props and state both?

Not always would we want to sync with the backend? So the latter would be better.

stitesExpensify commented 3 years ago

I am calling the following code in the AuthScreen. I am a little confused it should be Report page or AuthScreen?

I think we should do it here (it's just a comma separated list)

Should I maintain the preferredSkinTone in the EmojiPickerMenu state or props and state both?

We should only need the props right? So we get it from onyx on load, and then if we change the skin tone we set it in the server with nameValuePairs.set and set it in onyx with onyx.set which will reload our props for the picker right?

mananjadhav commented 3 years ago

I am calling the following code in the AuthScreen. I am a little confused it should be Report page or AuthScreen?

I think we should do it here (it's just a comma separated list)

Thanks.

Should I maintain the preferredSkinTone in the EmojiPickerMenu state or props and state both?

We should only need the props right? So we get it from onyx on load, and then if we change the skin tone we set it in the server with nameValuePairs.set and set it in onyx with onyx.set which will reload our props for the picker right?

Will keep props only then. So we would never have a case where we would use EmojiPicker skinTone change without updating in Onyx & NVP?

stitesExpensify commented 3 years ago

So we would never have a case where we would use EmojiPicker skinTone change without updating in Onyx & NVP?

Yep!We will always want to keep onyx and the NVP in sync with whatever the user chooses

puneetlath commented 3 years ago

@mananjadhav created the job and invited you to it here: https://www.upwork.com/jobs/~01c482f3ae46293cc6

shawnborton commented 3 years ago

Cc'ing @michelle-thompson here - it looks like we already have mockups that you made, but perhaps you can work with @mananjadhav on the PR for design feedback once it gets underway.

mananjadhav commented 3 years ago

@puneetlath Does the amount scope only the development effort? or does it include the bonus for adding the issue as well?

puneetlath commented 3 years ago

Hi @mananjadhav. That is for the development effort. We will also add the bonus for finding the issue upon payment. Let me know if you say any issues with that. Thanks!

mananjadhav commented 3 years ago

I am expecting a little more than what is offered, but my bad I didn't wait for your proposal. I am okay if you feel there isn't we can do about it. Leave it for you to decide, have accepted the proposal anyway.

Also, do you foresee any changes/enhancements expected from what is implemented?

puneetlath commented 3 years ago

@mananjadhav and I discussed the scope of the work offline and the job offer has been updated accordingly.

MelvinBot commented 3 years ago

This issue has not been updated in over 15 days. @puneetlath, @mananjadhav eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

mananjadhav commented 3 years ago

PR is updated and should get merged by today/tomorrow.

puneetlath commented 3 years ago

Paid!