feat: Dynamic emoji size #3561

pranshuchittora · 2021-06-11T20:02:15Z

Details

Fixed Issues

Fixes #2662

Tests

QA Steps

Check for emoji sizes in small devices.
Check for emoji overflow/clipping

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Smallest device possible (approx 320)

Medium Size WVGA

High DPI

pranshuchittora · 2021-06-11T20:03:59Z

@chiragsalian I am able to get this to work.

But the flatlist is not re-rendering and getting this weird effect on hover as the size changes are not being reflected.

Peek.2021-06-12.01-22.mp4

chiragsalian · 2021-06-14T16:34:12Z

src/pages/home/report/EmojiPickerMenu/dynamicEmojiSize.js

+        return styleVariables.fontSizeSmall;
+    }
+    if (
+        windowWidth <= 480


Let's convert this to a single line if condition for consistency. Its weird seeing a mix of single and double line if statements when they have just 1 condition in them.

chiragsalian · 2021-06-14T16:34:39Z

src/pages/home/report/EmojiPickerMenu/index.js

+
+    componentDidUpdate(prevProps) {
+        if (
+            this.props.windowWidth !== prevProps.windowWidth


Same as before, let's make this a single line if statement.

chiragsalian · 2021-06-14T16:37:24Z

src/pages/home/report/EmojiPickerMenu/index.js

+        if (
+            this.props.windowWidth !== prevProps.windowWidth
+        ) {
+            // eslint-disable-next-line react/no-did-update-set-state


Is there a reason you would prefer to disable this rule. IMO it feels just as easy to create a method like setDynamicEmojiSize and call that method here that would do,

this.setState({emojiSize: dynamicEmojiSize(this.props.windowWidth)});

Then you wouldn't need to disable the eslint rule.

chiragsalian · 2021-06-14T16:38:43Z

src/pages/home/report/EmojiPickerMenuItem.js

@@ -17,6 +17,8 @@ const propTypes = {

    /** Whether this menu item is currently highlighted or not */
    isHighlighted: PropTypes.bool.isRequired,
+
+    size: PropTypes.number.isRequired,


missing JS doc.

chiragsalian · 2021-06-14T16:39:11Z

src/pages/home/report/EmojiPickerMenu/index.native.js

    }

+


remove unnecessary line break

chiragsalian · 2021-06-14T16:39:28Z

src/pages/home/report/EmojiPickerMenu/index.native.js

+        if (
+            this.props.windowWidth !== prevProps.windowWidth
+        ) {
+            // eslint-disable-next-line react/no-did-update-set-state


Similar comment as here

chiragsalian · 2021-06-14T16:41:38Z

src/pages/home/report/EmojiPickerMenu/index.native.js


 const propTypes = {
    /** Function to add the selected emoji to the main compose text input */
    onEmojiSelected: PropTypes.func.isRequired,
+
+    ...windowDimensionsPropTypes,


NAB: I see there are many places in our code where this is prop is not documented but as per our style guide it would be nice if you include a JS comment doc for this for consistency.

pranshuchittora · 2021-06-14T17:07:30Z

@chiragsalian I have updated the PR

chiragsalian · 2021-06-14T17:23:29Z

But the flatlist is not re-rendering and getting this weird effect on hover as the size changes are not being reflected.

Just saw this. I was OOO on Friday so just getting to this today. I'll first ask @stitesExpensify opinion if he knows how to tackle this since he worked on this. Stites could you let us know if you know a fix for the problem he mentioned. He shared a video of the problem too which helps. If you have no idea then I'll pull the code and start debugging. (I'm a little swamped today hence was asking you first)

chiragsalian · 2021-06-14T17:34:44Z

Also i think you should be giving emoji size as a prop to EmojiPickerMenu/index.js because the way it is now, if you calculate and call setState on EmojiPickerMenu/index.js you'll be rerendering so many times within the emoji component (you can test this by logging the render and see that it'll be called almost infinitely). I definitely think we should avoid a setState if possible and I don't think we should care about the width changing dynamically for now so the problem you mentioned may be a non issue but I'll let @stitesExpensify share his thoughts on the same as well.

chiragsalian · 2021-06-14T17:46:10Z

src/pages/home/report/EmojiPickerMenu/dynamicEmojiSize.js

-    }
-
+    if (windowWidth <= 320) { return styleVariables.fontSizeSmall; }
+    if (windowWidth <= 480) { return styleVariables.fontSizeNormal; }


oh sorry for the confusion, i didn't mean the whole line, just the if condition, i.e.,

if (windowWidth <= 480) { return styleVariables.fontSizeNormal; }

That would match our examples in the style guide. Could you make the same changes to the if conditions in your file.

pranshuchittora · 2021-06-14T17:46:42Z

Also i think you should be giving emoji size as a prop to EmojiPickerMenu/index.js because the way it is now, if you calculate and call setState on EmojiPickerMenu/index.js you'll be rerendering so many times within the emoji component (you can test this by logging the render and see that it'll be called almost infinitely). I definitely think we should avoid a setState if possible and I don't think we should care about the width changing dynamically for now so the problem you mentioned may be a non issue but I'll let @stitesExpensify share his thoughts on the same as well.

Is this for me @chiragsalian

chiragsalian · 2021-06-14T19:16:26Z

I don't think we should care about the width changing dynamically for now so the problem you mentioned may be a non issue but I'll let @stitesExpensify share his thoughts on the same as well.

So just this part is for stites and the rest was for you. Also it looks like stites is OOO today so I guess now all of it is for you 🙂
Let me know your thoughts.

chiragsalian · 2021-06-14T20:18:29Z

Also i don't think there is any reason to make your changes to index.js. We don't have any issue with the way the emojis work on web so I think we should just focus on fixing it for mobile. So just having your code for index.native.js and testing it on android and ios for different mobile widths would suffice.

nickmurray47 · 2021-06-17T21:20:53Z

@pranshuchittora hey! I'm interested in pushing this along and wondering if there were any updates on your end. It looks like the issue is that we expect the extraData prop to help trigger a flatList re-render but instead the items only re-render when you hover over them. Does that sound accurate and have you found out anything additional?

pranshuchittora · 2021-06-18T13:05:29Z

@nickmurray47
Yep the items are not re-rendering even extraData changes. And only re-renders on hovering, I am not able to find any workaround.

chiragsalian · 2021-06-23T23:01:16Z

@pranshuchittora, so you mention re-rendering. But i don't see how the dimension would change on the mobile device such that we'd have to worry about re-renders (well landscape and portrait could exist but I think that's an improvement for a later date).
For now can't we detect whats the current width, if its less than a certain number pass it as a prop and then over here instead of styles.emojiText use a new style like styles.emojiTextSmall which has a smaller font.

Then test if its works for Huawei P20 Pro / Android 10 on a simulator and we should be good right?

pranshuchittora · 2021-06-28T17:07:29Z

src/pages/home/report/EmojiPickerMenu/index.js

+            emojiSize: {
+                fontSize: dynamicEmojiSize(this.props.windowWidth),
+            },


Why this?

To reduce the memory overhead of creating objects for each emoji.

pranshuchittora · 2021-06-28T17:15:45Z

@chiragsalian I have updated the PR

Smallest device possible (approx 320)

Medium Size WVGA

High DPI

…emoji-size

chiragsalian

Left a minor comment. The rest looks much better to me. I tested and it works well too.

chiragsalian · 2021-07-01T00:23:01Z

src/pages/home/report/EmojiPickerMenu/index.native.js

+
+        this.state = {
+            emojiSize: {fontSize: dynamicEmojiSize(this.props.windowWidth)},
+        };


Do you need this state variable? Its not really changing anywhere in the component so I don't quite see the benefit.
How about if you just did,

emojiSize={fontSize: dynamicEmojiSize(this.props.windowWidth)}

over here.

If you think that's not too readable then we should just do this.emojiSize than save it in a state variable. Thoughts?

The idea of keeping it in a state opens the door for any future development for reactivity for emoji sizes.
And adding it to the state doesn't create any overhead.

We can remove it from the state and make it a normal class property.

- this.state.emojiSize + this.emojiSize

The idea of keeping it in a state opens the door for any future development for reactivity for emoji sizes.

Making code more complex than it needs to be with the assumption that it will be modified in the future is unnecessary preoptimization. When its needed to be a state variable for any reason then its easy enough to convert so let's not have it be a state variable right now.

Sure I'm fine with class property 👍
Could you update?

Done I have updated the PR

pranshuchittora · 2021-07-01T18:30:31Z

I have updated the PR. Updating the screenshots and QA steps asynchronously.

chiragsalian

Nice, ty. Code LGTM 👍

chiragsalian · 2021-07-02T17:14:04Z

@nickmurray47 bump to review.

pranshuchittora · 2021-07-02T18:55:20Z

I have updated all the screenshots and QA steps

nickmurray47

Tests well and code LGTM

OSBotify · 2021-07-06T18:23:18Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

roryabraham · 2021-07-09T00:29:15Z

Unfortunately the deploy comments are not working right now. This was deployed to staging yesterday.

Jag96 · 2021-07-14T22:30:48Z

Commented in #4030 (comment) about a regression from this PR

feat: Dynamic emoji size

8baee4f

pranshuchittora requested a review from a team as a code owner June 11, 2021 20:02

MelvinBot requested review from nickmurray47 and removed request for a team June 11, 2021 20:02

chiragsalian requested changes Jun 14, 2021

View reviewed changes

pranshuchittora added 2 commits June 14, 2021 22:34

chore: Minor changes as per the PR review

7fa65be

chore: Minor js doc changes

1f1956c

pranshuchittora requested a review from chiragsalian June 14, 2021 17:07

chiragsalian requested changes Jun 14, 2021

View reviewed changes

fix: Minor styling changes

379b771

pranshuchittora added 2 commits June 27, 2021 18:08

feat: Emoji size fix

156fe87

feat: Updated logic

734ee3e

pranshuchittora commented Jun 28, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into pranshuchittora/feat/…

bcc0882

…emoji-size

pranshuchittora requested a review from chiragsalian June 28, 2021 17:48

chiragsalian requested changes Jul 1, 2021

View reviewed changes

fix: Minor PR changes

0b34771

pranshuchittora requested a review from chiragsalian July 1, 2021 18:30

chiragsalian approved these changes Jul 1, 2021

View reviewed changes

rdjuric mentioned this pull request Jul 2, 2021

[ON HOLD] Emoji selector - Focused emoji is not centered in focus area #3862

Closed

nickmurray47 approved these changes Jul 6, 2021

View reviewed changes

nickmurray47 merged commit e78340b into Expensify:main Jul 6, 2021

OSBotify mentioned this pull request Jul 7, 2021

Deploy Checklist: Expensify.cash 2021-07-07 #3905

Closed

57 tasks

parasharrajat mentioned this pull request Jul 14, 2021

Emoji - Emoji grid is not correctly laid out #4030

Closed

feat: Dynamic emoji size #3561

feat: Dynamic emoji size #3561

Conversation

pranshuchittora commented Jun 11, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

pranshuchittora commented Jun 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragsalian Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora commented Jun 14, 2021

chiragsalian commented Jun 14, 2021

chiragsalian commented Jun 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora commented Jun 14, 2021

chiragsalian commented Jun 14, 2021

chiragsalian commented Jun 14, 2021

nickmurray47 commented Jun 17, 2021

pranshuchittora commented Jun 18, 2021 • edited Loading

chiragsalian commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora commented Jun 28, 2021

chiragsalian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora commented Jul 1, 2021

chiragsalian left a comment

Choose a reason for hiding this comment

chiragsalian commented Jul 2, 2021

pranshuchittora commented Jul 2, 2021

nickmurray47 left a comment

Choose a reason for hiding this comment

OSBotify commented Jul 6, 2021

roryabraham commented Jul 9, 2021

Jag96 commented Jul 14, 2021

pranshuchittora commented Jun 11, 2021 •

edited

Loading

chiragsalian Jun 14, 2021 •

edited

Loading

pranshuchittora commented Jun 18, 2021 •

edited

Loading