[HOLD for payment 2023-02-06] [$1000] The emoji titles are not vertically centered in android #14462

kavimuru · 2023-01-22T18:20:25Z

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:

Open the app
Go to any chat
Open emoji picker
You'll notice that the emoji titles aren't vertically centered

Expected Result:

Emoji titles should be vertically centered

Actual Result:

They aren't vertically centered in Android

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Android / native
iOS / native

Version Number: 1.2.57-2
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Expensify/Expensify Issue URL:
Issue reported by: @daraksha-dk
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674393700959009

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c89c9325c04466a1
Upwork Job ID: 1618082297115369472
Last Price Increase: 2023-01-25

melvin-bot · 2023-01-22T18:20:31Z

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

greg-schroeder · 2023-01-25T02:12:12Z

greg-schroeder · 2023-01-25T02:16:22Z

Per discussion in Slack we're not sure if this is Bug or NewFeature: discussion

greg-schroeder · 2023-01-25T03:04:06Z

Seems like in this issue they're aligned, but that's Web. I mean, I think we should prob fix this anyway as it just looks off.

melvin-bot · 2023-01-25T03:04:19Z

Job added to Upwork: https://www.upwork.com/jobs/~01c89c9325c04466a1

melvin-bot · 2023-01-25T03:04:22Z

Current assignee @greg-schroeder is eligible for the External assigner, not assigning anyone new.

melvin-bot · 2023-01-25T03:04:31Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

Pujan92 · 2023-01-25T03:04:36Z

Proposal*

It seems we need to wrap the Text in the View(bcoz Text doesn't required 32px height) which will be the solution for this rendering issue(Text renders different in the native), and for better change and consistency we can apply the same for web too.

diff --git a/src/components/EmojiPicker/EmojiPickerMenu/index.js b/src/components/EmojiPicker/EmojiPickerMenu/index.js
index 924d81a2b..3994f17d4 100755
--- a/src/components/EmojiPicker/EmojiPickerMenu/index.js
+++ b/src/components/EmojiPicker/EmojiPickerMenu/index.js
@@ -438,9 +438,11 @@ class EmojiPickerMenu extends Component {
 
         if (header) {
             return (
-                <Text style={styles.emojiHeaderStyle}>
-                    {this.props.translate(`emojiPicker.headers.${code}`)}
-                </Text>
+                <View style={styles.emojiHeaderWrapper}>
+                    <Text style={styles.emojiHeaderText}>
+                        {this.props.translate(`emojiPicker.headers.${code}`)}
+                    </Text>
+                </View>
             );
         }
 
diff --git a/src/components/EmojiPicker/EmojiPickerMenu/index.native.js b/src/components/EmojiPicker/EmojiPickerMenu/index.nati
ve.js
index 754ed20fd..17a9d50e9 100644
--- a/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
+++ b/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
@@ -107,9 +107,11 @@ class EmojiPickerMenu extends Component {
 
         if (item.header) {
             return (
-                <Text style={styles.emojiHeaderStyle}>
-                    {this.props.translate(`emojiPicker.headers.${item.code}`)}
-                </Text>
+                <View style={styles.emojiHeaderWrapper}>
+                    <Text style={styles.emojiHeaderText}>
+                        {this.props.translate(`emojiPicker.headers.${item.code}`)}
+                    </Text>
+                </View>
             );
         }
 
diff --git a/src/styles/styles.js b/src/styles/styles.js
index e30b5f77b..6077e7f47 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -1459,12 +1459,16 @@ const styles = {
         height: 240,
     },
 
-    emojiHeaderStyle: {
+    emojiHeaderWrapper: {
         backgroundColor: themeColors.componentBG,
         width: '100%',
         height: 32,
         display: 'flex',
+        flexDirection: 'row',
         alignItems: 'center',
+    },
+
+    emojiHeaderText: {
         fontFamily: fontFamily.EXP_NEUE_BOLD,
         fontWeight: fontWeightBold,
         color: themeColors.heading,

melvin-bot · 2023-01-25T03:04:36Z

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

daraksha-dk · 2023-01-25T03:04:51Z

Proposal

This is a regression caused by this PR (Must have been ignored while testing). In order to fix this we can make the change specifically in the native file by adding a Parent container or we can have the same changes done for both files like shown below (the latter approach seems better for consistency).

Here is the code change that will be required

diff --git a/src/components/EmojiPicker/EmojiPickerMenu/index.js b/src/components/EmojiPicker/EmojiPickerMenu/index.js
index 87e3356b7..2200af945 100755
--- a/src/components/EmojiPicker/EmojiPickerMenu/index.js
+++ b/src/components/EmojiPicker/EmojiPickerMenu/index.js
@@ -446,9 +446,11 @@ class EmojiPickerMenu extends Component {
 
         if (header) {
             return (
-                <Text style={styles.emojiHeaderStyle}>
-                    {this.props.translate(`emojiPicker.headers.${code}`)}
-                </Text>
+                <View style={styles.emojiHeaderContainer}>
+                    <Text style={styles.emojiHeaderStyle}>
+                        {this.props.translate(`emojiPicker.headers.${code}`)}
+                    </Text>
+                </View>
             );
         }
 
diff --git a/src/components/EmojiPicker/EmojiPickerMenu/index.native.js b/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
index 754ed20fd..d70eed988 100644
--- a/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
+++ b/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
@@ -107,9 +107,11 @@ class EmojiPickerMenu extends Component {
 
         if (item.header) {
             return (
-                <Text style={styles.emojiHeaderStyle}>
-                    {this.props.translate(`emojiPicker.headers.${item.code}`)}
-                </Text>
+                <View style={styles.emojiHeaderContainer}>
+                    <Text style={styles.emojiHeaderStyle}>
+                        {this.props.translate(`emojiPicker.headers.${item.code}`)}
+                    </Text>
+                </View>
             );
         }
 
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 71fed0201..a8a0bb36e 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -1459,12 +1459,14 @@ const styles = {
         height: 240,
     },
 
-    emojiHeaderStyle: {
+    emojiHeaderContainer: {
         backgroundColor: themeColors.componentBG,
-        width: '100%',
-        height: 32,
         display: 'flex',
-        alignItems: 'center',
+        height: 32,
+        justifyContent: 'center',
+        width: '100%',
+    },
+    emojiHeaderStyle: {
         fontFamily: fontFamily.EXP_NEUE_BOLD,
         fontWeight: fontWeightBold,
         color: themeColors.heading,

Demo

emoji-header.mp4

Pujan92 · 2023-01-25T03:18:25Z

This is a regression caused by this PR

Yes, the removal of vertical padding and applied height without wrapper caused this. Thanks for the find.

jatinsonijs · 2023-01-25T04:01:15Z

Root Case:
We are applying height and align center on Text component but align center work for children where it is apply not for itself.

Occurred after these changes
PR: #13787
Issue: #12772

Proposal
We should also use const value of emoji header height as we already have const for emoji header height to make it consistent we should use const.

diff --git a/src/components/EmojiPicker/EmojiPickerMenu/index.js b/src/components/EmojiPicker/EmojiPickerMenu/index.js
index 924d81a2bf..e2ad3bed1b 100755
--- a/src/components/EmojiPicker/EmojiPickerMenu/index.js
+++ b/src/components/EmojiPicker/EmojiPickerMenu/index.js
@@ -438,9 +438,11 @@ class EmojiPickerMenu extends Component {
 
         if (header) {
             return (
-                <Text style={styles.emojiHeaderStyle}>
-                    {this.props.translate(`emojiPicker.headers.${code}`)}
-                </Text>
+                <View style={styles.emojiHeaderContainer}>
+                    <Text style={styles.emojiHeaderStyle}>
+                        {this.props.translate(`emojiPicker.headers.${code}`)}
+                    </Text>
+                </View>
             );
         }
 
diff --git a/src/components/EmojiPicker/EmojiPickerMenu/index.native.js b/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
index 754ed20fd7..d70eed9880 100644
--- a/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
+++ b/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
@@ -107,9 +107,11 @@ class EmojiPickerMenu extends Component {
 
         if (item.header) {
             return (
-                <Text style={styles.emojiHeaderStyle}>
-                    {this.props.translate(`emojiPicker.headers.${item.code}`)}
-                </Text>
+                <View style={styles.emojiHeaderContainer}>
+                    <Text style={styles.emojiHeaderStyle}>
+                        {this.props.translate(`emojiPicker.headers.${item.code}`)}
+                    </Text>
+                </View>
             );
         }
 
diff --git a/src/styles/styles.js b/src/styles/styles.js
index e30b5f77b5..7f8906fe3f 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -1459,12 +1459,14 @@ const styles = {
         height: 240,
     },
 
-    emojiHeaderStyle: {
+    emojiHeaderContainer: {
         backgroundColor: themeColors.componentBG,
         width: '100%',
-        height: 32,
+        height: CONST.EMOJI_PICKER_HEADER_HEIGHT,
         display: 'flex',
-        alignItems: 'center',
+        justifyContent: 'center',
+    },
+    emojiHeaderStyle: {
         fontFamily: fontFamily.EXP_NEUE_BOLD,
         fontWeight: fontWeightBold,
         color: themeColors.heading,

Pujan92 · 2023-01-25T08:38:41Z

It seems we need to wrap the Text in the View(bcoz Text doesn't required 32px height) which will be the solution for this rendering issue(Text renders different in the native), and for better change and consistency we can apply the same for web too.

SS

thesahindia · 2023-01-25T18:07:15Z

The first two proposals were posted at the same time and both are similar. I prefer @daraksha-dk's proposal because they researched and added the PR that caused this regression in their proposal.

C+ reviewed 🎀👀🎀

cc: @pecanoro for the final decision

Pujan92 · 2023-01-25T18:09:19Z

@thesahindia sorry but It was already pointed here earlier.

Not sure whether that should be considered as an advantage over order.

jatinsonijs · 2023-01-25T18:09:36Z

@thesahindia If it is about PR caused regression I have already reported it in slack thread.

jatinsonijs · 2023-01-25T18:10:32Z

https://expensify.slack.com/archives/C049HHMV9SM/p1674566201027659?thread_ts=1674393700.959009&cid=C049HHMV9SM

jatinsonijs · 2023-01-25T18:17:34Z

I think we all already have proposal for this sol, but there is not enough time as proposal was posted immediately, I have just added one extra thing height: CONST.EMOJI_PICKER_HEADER_HEIGHT, and first who posted the regression PR.

thesahindia · 2023-01-25T18:20:28Z

Not sure whether that should be considered as an advantage over order.

Both proposals were added at the same time (8:34) so it's only a matter of preference, since I saw @daraksha-dk also added the PR that caused the regression I preferred them. The final decision will be from @pecanoro.

@thesahindia If it is about PR caused regression I have already reported it in slack thread.

Your solution was 1 hour late so the decision can't be taken based on that.

jatinsonijs · 2023-01-25T18:22:29Z

Its seems like race we have to post immediately after External tag added 🙂, Is using height from constant does not matter here ?

Your solution was 1 hour late so the decision can't be taken based on that.

yes earlier I have the same solution like others, so I have just tried to find out what further we can do better

jatinsonijs · 2023-01-25T18:58:13Z

Understood @thesahindia, @Pujan92 right it can happen for these type of issues so what we can improve here is for straightforward solutions - small enhance/improve should be consider if it is valid for betterment.

pecanoro · 2023-01-25T23:09:40Z

Assigning @daraksha-dk to the issue. Another reason to go with @daraksha-dk is that the regression was introduced by @Pujan92 so I think it would be unfair to get paid for fixing your own bug 😅

melvin-bot · 2023-01-25T23:10:04Z

📣 @daraksha-dk You have been assigned to this job by @pecanoro!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

Pujan92 · 2023-01-26T02:13:09Z

Assigning @daraksha-dk to the issue. Another reason to go with @daraksha-dk is that the regression was introduced by @Pujan92 so I think it would be unfair to get paid for fixing your own bug 😅

@pecanoro I would disagree over here, I was thought and asked for direct PR in slack as this issue caused due to my PR. IMO As this won't be considered a regression now, I am allowed to propose a solution to this fresh issue. cc: @JmillsExpensify

As it is already assigned I am ok with it but just wanted to make my point, Thanks.

asAnyOne · 2023-01-26T08:19:59Z

Why don't to add just a style to emojiHeaderStyle like this?

daraksha-dk · 2023-01-26T09:47:44Z

PR is ready @thesahindia

melvin-bot · 2023-01-30T18:29:15Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2023-01-30T18:29:16Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.61-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

Fix emoji header alignment #14586

If no regressions arise, payment will be issued on 2023-02-06. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-01-30T18:29:18Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@thesahindia / @pecanoro] The PR that introduced the bug has been identified. Link to the PR: Prevent emoji Picker list jumping while scrolling through arrow keys #13787
[@thesahindia / @pecanoro] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: Prevent emoji Picker list jumping while scrolling through arrow keys #13787 (comment)
[@thesahindia / @pecanoro] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: https://expensify.slack.com/archives/C049HHMV9SM/p1675710681694509
[@greg-schroeder] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

thesahindia · 2023-02-06T19:13:09Z

The PR that introduced the bug has been identified. Link to the PR:

#13787

The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

#13787 (comment)

A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

https://expensify.slack.com/archives/C049HHMV9SM/p1675710681694509

thesahindia · 2023-02-06T19:15:36Z

@greg-schroeder, I have completed the first three tasks. All just have to add a regression test. I think these steps will do the job -

Open the app
Go to any chat
Open emoji picker
Verify that emoji titles are vertically centered

thesahindia · 2023-02-06T19:16:04Z

@greg-schroeder, it is also ready for the payment.

greg-schroeder · 2023-02-07T21:11:19Z

Was OOO yesterday, dealing with payment and regression steps today!

greg-schroeder · 2023-02-07T23:15:35Z

Issue reporter: @daraksha-dk ($250)
Contributor: @daraksha-dk ($1000)
PR Reviewer: @thesahindia ($1000)

PR merged within 3 business days, eligible for 50% bonus

greg-schroeder · 2023-02-07T23:19:19Z

Sent offers to you both - please accept and I'll take care of payment + bonus

thesahindia · 2023-02-08T05:20:45Z

Accepted, thanks!

daraksha-dk · 2023-02-08T05:48:47Z

Offer accepted, thanks!

greg-schroeder · 2023-02-08T06:13:59Z

Paid, job closed.

greg-schroeder · 2023-02-08T08:03:25Z

Filed regression test request per #14462 (comment)

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 22, 2023

melvin-bot bot assigned greg-schroeder Jan 22, 2023

melvin-bot bot locked and limited conversation to collaborators Jan 22, 2023

greg-schroeder added the External Added to denote the issue can be worked on by a contributor label Jan 25, 2023

melvin-bot bot unlocked this conversation Jan 25, 2023

melvin-bot bot changed the title ~~The emoji titles are not vertically centered in android~~ [$1000] The emoji titles are not vertically centered in android Jan 25, 2023

melvin-bot bot assigned thesahindia Jan 25, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 25, 2023

melvin-bot bot assigned pecanoro Jan 25, 2023

pecanoro assigned daraksha-dk Jan 25, 2023

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 25, 2023

daraksha-dk mentioned this issue Jan 26, 2023

Fix emoji header alignment #14586

Merged

54 tasks

melvin-bot bot added the Reviewing Has a PR in review label Jan 26, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jan 30, 2023

melvin-bot bot changed the title ~~[$1000] The emoji titles are not vertically centered in android~~ [HOLD for payment 2023-02-06] [$1000] The emoji titles are not vertically centered in android Jan 30, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Jan 30, 2023

thesahindia mentioned this issue Feb 6, 2023

Prevent emoji Picker list jumping while scrolling through arrow keys #13787

Merged

53 tasks

greg-schroeder closed this as completed Feb 8, 2023

[HOLD for payment 2023-02-06] [$1000] The emoji titles are not vertically centered in android #14462

[HOLD for payment 2023-02-06] [$1000] The emoji titles are not vertically centered in android #14462

Comments

kavimuru commented Jan 22, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Jan 22, 2023

greg-schroeder commented Jan 25, 2023 • edited Loading

Bug0 Triage Checklist

greg-schroeder commented Jan 25, 2023 • edited Loading

greg-schroeder commented Jan 25, 2023

melvin-bot bot commented Jan 25, 2023

melvin-bot bot commented Jan 25, 2023

melvin-bot bot commented Jan 25, 2023

Pujan92 commented Jan 25, 2023

melvin-bot bot commented Jan 25, 2023

daraksha-dk commented Jan 25, 2023

Proposal

Demo

Pujan92 commented Jan 25, 2023

jatinsonijs commented Jan 25, 2023 • edited Loading

Pujan92 commented Jan 25, 2023

thesahindia commented Jan 25, 2023

Pujan92 commented Jan 25, 2023 • edited Loading

jatinsonijs commented Jan 25, 2023

jatinsonijs commented Jan 25, 2023

jatinsonijs commented Jan 25, 2023 • edited Loading

thesahindia commented Jan 25, 2023

jatinsonijs commented Jan 25, 2023 • edited Loading

jatinsonijs commented Jan 25, 2023

pecanoro commented Jan 25, 2023

melvin-bot bot commented Jan 25, 2023

Pujan92 commented Jan 26, 2023

asAnyOne commented Jan 26, 2023 • edited Loading

daraksha-dk commented Jan 26, 2023

melvin-bot bot commented Jan 30, 2023

melvin-bot bot commented Jan 30, 2023

melvin-bot bot commented Jan 30, 2023 • edited by greg-schroeder Loading

thesahindia commented Feb 6, 2023 • edited Loading

thesahindia commented Feb 6, 2023

thesahindia commented Feb 6, 2023

greg-schroeder commented Feb 7, 2023 • edited Loading

greg-schroeder commented Feb 7, 2023

greg-schroeder commented Feb 7, 2023

thesahindia commented Feb 8, 2023

daraksha-dk commented Feb 8, 2023

greg-schroeder commented Feb 8, 2023

greg-schroeder commented Feb 8, 2023

kavimuru commented Jan 22, 2023 •

edited by melvin-bot bot

Loading

greg-schroeder commented Jan 25, 2023 •

edited

Loading

greg-schroeder commented Jan 25, 2023 •

edited

Loading

jatinsonijs commented Jan 25, 2023 •

edited

Loading

Pujan92 commented Jan 25, 2023 •

edited

Loading

jatinsonijs commented Jan 25, 2023 •

edited

Loading

jatinsonijs commented Jan 25, 2023 •

edited

Loading

asAnyOne commented Jan 26, 2023 •

edited

Loading

melvin-bot bot commented Jan 30, 2023 •

edited by greg-schroeder

Loading

thesahindia commented Feb 6, 2023 •

edited

Loading

greg-schroeder commented Feb 7, 2023 •

edited

Loading