[PAID] [$1000] Padding below 'change default skin tone' increases on no result found in emoji picker #15868

kavimuru · 2023-03-10T23:42:46Z

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
Open any report
Open emoji picker
Search for any emoji eg: 'skull' and observe padding below 'change default skin tone'
Search for emoji that does not exist eg: 'skulll' and observe padding below 'change default skin tone'

Expected Result:

Padding below 'change default skin tone' should be same all the time

Actual Result:

Padding below 'change default skin tone' increases on no result found search in emoji picker

Workaround:

unknown

Platforms:

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

Version Number: 1.2.82-3
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:

padding.below.skin.tone.mp4

Recording.1680.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1678467171785959

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~019f078af8705c66b2
Upwork Job ID: 1636362309916753920
Last Price Increase: 2023-03-16

MelvinBot · 2023-03-10T23:42:52Z

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

MelvinBot · 2023-03-10T23:42:55Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

MelvinBot · 2023-03-14T21:02:40Z

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

MelvinBot · 2023-03-16T13:42:33Z

Job added to Upwork: https://www.upwork.com/jobs/~019f078af8705c66b2

MelvinBot · 2023-03-16T13:42:37Z

Current assignee @anmurali is eligible for the External assigner, not assigning anyone new.

MelvinBot · 2023-03-16T13:42:45Z

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

MelvinBot · 2023-03-16T13:42:51Z

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

PrashantMangukiya · 2023-03-16T13:42:57Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Padding below 'change default skin tone' increases on no result found in emoji picker.

What is the root cause of that problem?

It is rendering "No results found" message via <Text> component at line 501

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Lines 499 to 513 in b69b5f3

    
           {this.state.filteredEmojis.length === 0 
        
               ? ( 
        
                   <Text 
        
                       style={[ 
        
                           styles.disabledText, 
        
                           styles.emojiPickerList, 
        
                           styles.dFlex, 
        
                           styles.alignItemsCenter, 
        
                           styles.justifyContentCenter, 
        
                           this.isMobileLandscape() && styles.emojiPickerListLandscape, 
        
                       ]} 
        
                   > 
        
                       {this.props.translate('common.noResultsFound')} 
        
                   </Text> 
        
               )

We can see that styles.dFlex is there but styles.flex1 missing, so whenever "No results found" message render at that time it will not occupy all space and so due to that <EmojiSkinToneList> component (it is below that) slightly jump up words.

What changes do you think we should make in order to solve the problem?

Within src/components/EmojiPicker/EmojiPickerMenu/index.js we have to add styles.flex1 for "No results found" Text component as shown below, It will solve the problem. I tested it locally, It works as expected and solve this issue.

<Text
  style={[
      styles.disabledText,
      styles.emojiPickerList,
      styles.dFlex,
      styles.flex1,   // *** ADD THIS ***
      styles.alignItemsCenter,
      styles.justifyContentCenter,
      this.isMobileLandscape() && styles.emojiPickerListLandscape,
  ]}
>
    {this.props.translate('common.noResultsFound')}
</Text>

What alternative solutions did you explore? (Optional)

None

tienifr · 2023-03-16T13:43:04Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

The position of the 'change default skin tone' text change when no result is found in the emoji picker

What is the root cause of that problem?

If no result is found when searching for emoji, the No results found Text will appear instead of CategoryShortcutBar Component and Emoji List. But currently, the height of the No results found Text (288px) is smaller than the total height of the Emoji List (using flex-grow: 1 --> 256px) and CategoryShortcutBar Component (fixed --> 40px). So If no result is found when searching the 'change default skin tone' text will move up a bit

What changes do you think we should make in order to solve the problem?

We could add the flexGrow: 1 into the No results found Text instead of setting a fixed height for it

Result

Screen.Recording.2023-03-13.at.14.44.03.mov

abdulrahuman5196 · 2023-03-16T13:43:15Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

On emoji picker screen 'change default skin tone' button jumps up when we have 'No results found screen'.

What is the root cause of that problem?

This issue(#15868) seems to be a regression from #15730
We have increased the height of the emoji component from 392 to 416 i.e 416-392 = 24 new space
And increased skin tone padding from p1 to p3 i.e 16 space used
But we have not updated the height of the 'No results' component similarly here - https://github.com/Expensify/App/blob/main/src/components/EmojiPicker/EmojiPickerMenu/index.js#L501 leading to 8 unused space jumping up when we show no results component

What changes do you think we should make in order to solve the problem?

Option 1: Currently we show the 'no results component' for only big screen web devices in which case the emoji picker component has 416 height. So instead of hardcoding height of 'No results component', we can add styles.h100 here - https://github.com/Expensify/App/blob/main/src/components/EmojiPicker/EmojiPickerMenu/index.js#L507. This way 'No results' component will automatically adjust to the height of emoji picker and we don't need to update the height value of 'No results' component everytime we update the picker height.

Option 2: We can hardcode the height as 296(currently 288 + 8 extra left space) for the 'No result component'.

Option 3 (Extended option to solve the small screen issue - #15868 (comment)):
There is an another underlaying problem to the emojis picker to have difference in case of small screen devices and desktops devices.

Giving a small background on that,

As you can see the number of emoji rows we show in desktops is 7 and in small screen devices we show 8 but we give the flatlist a size of 288 in both the cases here - https://github.com/Expensify/App/blob/main/src/styles/styles.js#L1531.

The reason is for this we don't give predefined height in case of emoji picker shown on smallscreenwidth sizes. (Not sure on why we didn't give though)

App/src/styles/StyleUtils.js

Lines 456 to 467 in 885de7e

    
           function getEmojiPickerStyle(isSmallScreenWidth) { 
        
               if (isSmallScreenWidth) { 
        
                   return { 
        
                       width: '100%', 
        
                   }; 
        
               } 
        
               return { 
        
                   width: CONST.EMOJI_PICKER_SIZE.WIDTH, 
        
                   height: CONST.EMOJI_PICKER_SIZE.HEIGHT, 
        
               }; 
        
           }

So what happens when we don't give a predefined height is that the height of the emoji picker is calculated as the sum of its child components.
For example = FlatList(288) + skin tone component + etc

To the contrast in big screen devices the flatlist takes up 256px space only which is the only amount space left after other components take up from the predefined emoji component height of 416px

Coming back to the problem of 'No results' component having issues if we give 'h100' is that there is no predefined height for the component to grow into. So in case of small screen width sizes, if we wanted to show the 'No results' component to cope up correctly,

we have to set proper hardcoded value for 'No results' component
If we want 'No results' component to auto adjust, we have to provide predefined height to the emoji picker component(for smallScreenWidth as well) which is the sum of existing heights of child components and then provide auto-adjusting style to 'No results' component(h100, flex, flexgrow and any other would suffice). It won't have any behaviour differences as to current user experience and would auto grow even if we change the padding in futureBest IMO

Working video:

Screen.Recording.2023-03-15.at.11.31.45.PM.mov

What alternative solutions did you explore? (Optional)

There are multiple ways to achieve the result by modifying different styles but the above seems the best and future proof.

stitesExpensify · 2023-03-16T13:43:16Z

FYI to contributors there has already been a proposal to fix this regression by @abdulrahuman5196 here, which will be prioritized #15730 (comment)

Pujan92 · 2023-03-16T13:45:06Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Height inconsistency for Not Result Found in Emoji Picker

What is the root cause of that problem?

Incorrect height is given for the noResultsFound Text, It is given currently 288px which should be 256px(for 8 rows of emoji flatlist) + 40px(for CategoryShortcutBar)

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Line 504 in 20d5fdc

styles.emojiPickerList,

App/src/styles/styles.js

Lines 1497 to 1498 in 20d5fdc

    
           emojiPickerList: { 
        
               height: 288,

What changes do you think we should make in order to solve the problem?

We can add style styles.flex1 here which will grow with the available height for the notFound Text div element.

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Line 505 in 8a04b2c

styles.dFlex,

Extra: For Web we need 256px(for 8 rows * 32) height for the emoji flatlist and currently we have 288px, in native we are showing 9 rows where it would be required to 288px. So either we can maintain separate class for web & native OR in native also we can show 8 rows which we can achieve by general change of height to 256px in class emojiPickerList.

MelvinBot · 2023-03-16T21:04:17Z

@iwiznia, @anmurali, @rushatgabhane Eep! 4 days overdue now. Issues have feelings too...

MelvinBot · 2023-03-20T18:23:10Z

@iwiznia, @anmurali, @rushatgabhane 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

anmurali · 2023-03-20T19:17:12Z

Posted here to make a decision here

rushatgabhane · 2023-03-21T16:31:42Z

Proposal #15868 (comment)

Option 1: add styles.h100

That will break the emoji screen when we reduce the the screen size.

Option 2: We can hardcode the height as 296(currently 288 + 8 extra left space) for the 'No result component'.

I like option 2 the best.
But I have a concern - What if we change the padding again in future. Is there a solution that will auto adjust the height?

rushatgabhane · 2023-03-21T16:33:01Z

@abdulrahuman5196 i agree with the root cause.

but i don't agree with the problem saying that this is a regression from X PR.
Could you please restate the problem?

MelvinBot · 2023-03-31T10:43:11Z

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

MelvinBot · 2023-03-31T10:43:14Z

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

Fix/15868: the position of 'change default skin tone' change when no result found appear in emoji picker #16439

If no regressions arise, payment will be issued on 2023-04-07. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

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

MelvinBot · 2023-03-31T10:43:17Z

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:

[@rushatgabhane / @iwiznia] The PR that introduced the bug has been identified. Link to the PR:
[@rushatgabhane / @iwiznia] 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:
[@rushatgabhane / @iwiznia] 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:
[@anmurali] Determine if we should create a regression test for this bug.
[@rushatgabhane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

dhanashree-sawant · 2023-04-10T02:24:56Z

Hi @rushatgabhane, @iwiznia , @anmurali , ping for payment.

MelvinBot · 2023-04-10T09:36:54Z

@iwiznia, @anmurali, @rushatgabhane, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

MelvinBot · 2023-04-10T10:06:35Z

@iwiznia, @anmurali, @rushatgabhane, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

anmurali · 2023-04-10T22:39:27Z

@rushatgabhane can you take care of your items in the checklist? I think I pay and add regression tests last.

rushatgabhane · 2023-04-11T10:51:41Z

The PR that introduced the bug has been identified. Link to the PR: Add padding to skin tone picker #15730
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: https://github.com/Expensify/App/pull/15730/files#r1162636395
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:
No dicussion needed imo. It was a missed edge case.
If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
This bug won't happen again because of the changes made by @tienifr. No regression tests needed.

iwiznia · 2023-04-14T16:50:41Z

So what is missing here? Payment with the regression taken into consideration @anmurali?

anmurali · 2023-04-14T17:41:17Z

@tienifr - I cannot find you on Upwork. Your Slack and GH both don't list your name. Can you post it here so I know how to find you to pay you?

iwiznia · 2023-04-17T22:53:57Z

Bump @tienifr

tienifr · 2023-04-18T03:34:20Z

sorry missed this, @anmurali here's my Upwork profile https://www.upwork.com/freelancers/~01991fd5e5c11ef3ba, thanks

anmurali · 2023-04-18T21:09:22Z

Offers sent

anmurali · 2023-04-19T15:18:10Z

Everyone's paid now!

tienifr · 2023-05-02T14:52:29Z

hi @anmurali The PR is merged within 3 days of assignment so unless I'm missing something, this should be eligible for the speed bonus 🙇‍♂️.

tienifr · 2023-05-10T02:52:06Z

hi @anmurali The PR is merged within 3 days of assignment so unless I'm missing something, this should be eligible for the speed bonus 🙇‍♂️.

@anmurali can you help take a look? Thanks!

tienifr · 2023-05-23T12:46:27Z

hi @anmurali The PR is merged within 3 days of assignment so unless I'm missing something, this should be eligible for the speed bonus 🙇‍♂️.

@anmurali could you help to take a look at this?

@iwiznia just wondering if you can help here if @anmurali is unavailable?

Thanks!

anmurali · 2023-05-24T10:31:48Z

Done. @tienifr if you have asked her once and haven't received an answer in a week perhaps you can ping in Slack and tag the person you are waiting on instead?

tienifr · 2023-05-24T10:36:13Z

Thanks!

Done. @tienifr if you have asked her once and haven't received an answer in a week perhaps you can ping in Slack and tag the person you are waiting on instead?

@anmurali sure I'll take note of that, sorry for any inconvenience 🙇

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

melvin-bot bot assigned anmurali Mar 10, 2023

melvin-bot bot locked and limited conversation to collaborators Mar 10, 2023

melvin-bot bot added the Overdue label Mar 13, 2023

abdulrahuman5196 mentioned this issue Mar 15, 2023

Add padding to skin tone picker #15730

Merged

55 tasks

stitesExpensify added the External Added to denote the issue can be worked on by a contributor label Mar 16, 2023

melvin-bot bot unlocked this conversation Mar 16, 2023

melvin-bot bot changed the title ~~Padding below 'change default skin tone' increases on no result found in emoji picker~~ [$1000] Padding below 'change default skin tone' increases on no result found in emoji picker Mar 16, 2023

melvin-bot bot assigned rushatgabhane Mar 16, 2023

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

melvin-bot bot assigned iwiznia Mar 16, 2023

melvin-bot bot removed the Overdue label Mar 20, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 7, 2023

melvin-bot bot added the Overdue label Apr 10, 2023

melvin-bot bot removed the Overdue label Apr 10, 2023

melvin-bot bot added the Overdue label Apr 14, 2023

melvin-bot bot removed the Overdue label Apr 14, 2023

melvin-bot bot added the Overdue label Apr 17, 2023

melvin-bot bot removed the Overdue label Apr 17, 2023

anmurali closed this as completed Apr 19, 2023

anmurali removed the Awaiting Payment Auto-added when associated PR is deployed to production label Apr 19, 2023

anmurali changed the title ~~[HOLD for payment 2023-04-07] [$1000] Padding below 'change default skin tone' increases on no result found in emoji picker~~ [PAID] [$1000] Padding below 'change default skin tone' increases on no result found in emoji picker Apr 19, 2023

[PAID] [$1000] Padding below 'change default skin tone' increases on no result found in emoji picker #15868

[PAID] [$1000] Padding below 'change default skin tone' increases on no result found in emoji picker #15868

Comments

kavimuru commented Mar 10, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

MelvinBot commented Mar 10, 2023

MelvinBot commented Mar 10, 2023

Bug0 Triage Checklist (Main S/O)

MelvinBot commented Mar 14, 2023

MelvinBot commented Mar 16, 2023

MelvinBot commented Mar 16, 2023

MelvinBot commented Mar 16, 2023

MelvinBot commented Mar 16, 2023

PrashantMangukiya commented Mar 16, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

tienifr commented Mar 16, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Result

abdulrahuman5196 commented Mar 16, 2023 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

stitesExpensify commented Mar 16, 2023

Pujan92 commented Mar 16, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

MelvinBot commented Mar 16, 2023

MelvinBot commented Mar 20, 2023

anmurali commented Mar 20, 2023

rushatgabhane commented Mar 21, 2023 • edited Loading

rushatgabhane commented Mar 21, 2023

MelvinBot commented Mar 31, 2023

MelvinBot commented Mar 31, 2023

MelvinBot commented Mar 31, 2023 • edited by anmurali Loading

dhanashree-sawant commented Apr 10, 2023

MelvinBot commented Apr 10, 2023

MelvinBot commented Apr 10, 2023

anmurali commented Apr 10, 2023

rushatgabhane commented Apr 11, 2023 • edited Loading

iwiznia commented Apr 14, 2023

anmurali commented Apr 14, 2023

iwiznia commented Apr 17, 2023

tienifr commented Apr 18, 2023

anmurali commented Apr 18, 2023

anmurali commented Apr 19, 2023

tienifr commented May 2, 2023

tienifr commented May 10, 2023

tienifr commented May 23, 2023

anmurali commented May 24, 2023

tienifr commented May 24, 2023

kavimuru commented Mar 10, 2023 •

edited by melvin-bot bot

Loading

abdulrahuman5196 commented Mar 16, 2023 •

edited

Loading

rushatgabhane commented Mar 21, 2023 •

edited

Loading

MelvinBot commented Mar 31, 2023 •

edited by anmurali

Loading

rushatgabhane commented Apr 11, 2023 •

edited

Loading