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
2.99k stars 2.5k forks source link

[Payment due meow?][$1000] Update the emoji font library for Windows #21923

Closed kavimuru closed 5 months ago

kavimuru commented 10 months 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:

  1. Open the app
  2. Go to any chat > click on emoji
  3. Copy emojis and scroll down
  4. You'll notice that emoji header hasn't covered some remaining space on the right

    Expected Result:

    No space should be visible on the right side of the emoji header.

    Actual Result:

    Some space is visible on the right side.

    Deliverable:

    Update the emoji font library for Windows

Platforms:

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

Version Number: 1.3.34-1 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: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/e8d5a2d1-10c2-4519-be84-6194ac481ddb

https://github.com/Expensify/App/assets/43996225/b9bfe429-54fb-4121-a783-903772daf142

Expensify/Expensify Issue URL: Issue reported by: @aman-atg Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687788512893349

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013bc36e1574b8dee0
  • Upwork Job ID: 1680007465277968384
  • Last Price Increase: 2023-10-18
melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

samh-nl commented 10 months ago

Proposal

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

The emoji header does not fully extend to the right.

What is the root cause of that problem?

Some emojis are wider than the 12.5% width that is allocated to them, causing an overflow. The header has a padding-right applied to it, causing a gap that allows these extra wide emojis to get in-between.

https://github.com/Expensify/App/blob/0bbf3fccfce0994ddf4fd9e95fb33dd9edbd1868/src/styles/StyleUtils.js#L1151-L1155

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

There are two options:

Fixed result:

https://github.com/Expensify/App/assets/137707942/70682fdb-dcd7-49ac-ab03-9a3178c01646

What alternative solutions did you explore? (Optional)

Alternative 1: Hiding the overflow (or specifically overflow on x-axis) of the emojis to ensure that extra wide emojis are contained. Alternative 2: Allocate the appropriate width for each emoji, however this will cause misalignment problems (the emojis are displayed in a fixed cell-width grid for neat alignment).

melvin-bot[bot] commented 10 months ago

📣 @samh-nl! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
samh-nl commented 10 months ago

Contributor details Your Expensify account email: h.sam.lw@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~016c982fe69f86aff8

melvin-bot[bot] commented 10 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

@mallenexpensify Eep! 4 days overdue now. Issues have feelings too...

mallenexpensify commented 10 months ago

@aman-atg unable to reproduce in Chrome or Safari, should I be doing something differently?

Safari

https://github.com/Expensify/App/assets/22508304/ed7cfe12-80bb-4c68-bf87-85a8001a819c

Chrome

https://github.com/Expensify/App/assets/22508304/253f73b2-c846-44ef-aa09-4d0b7313454f

aman-atg commented 10 months ago

@aman-atg unable to reproduce in Chrome or Safari, should I be doing something differently?

Same situation for me in mac, but I can reproduce it on Linux (Chrome).

mallenexpensify commented 10 months ago

Kicking the can a bit here as it's not high value, will attempt to test again via browserstack later this week. Thanks for the update/reply @aman-atg

aman-atg commented 10 months ago

This issue is reproducible on Windows too, just wanted to point out as it might aid in testing.

mallenexpensify commented 10 months ago

Having issues access windows web via browserstack, asked internally about it here

melvin-bot[bot] commented 10 months ago

@mallenexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 10 months ago

Job added to Upwork: https://www.upwork.com/jobs/~013bc36e1574b8dee0

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

mallenexpensify commented 10 months ago

Was able to reproduce via Windows on Browserstack. Low value but reckon we should fix.

image
s-alves10 commented 10 months ago

Proposal

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

The emoji header is not taking full width in Emoji picker

What is the root cause of that problem?

This issue looks like happens only on Windows. The first reason for this issue is the windows scrollbar. On Mac, scrollbar doesn't affect the element size, but this is not the case on Windows. See below image.

Emoji picker modal has 320px width and 16px padding, so the content width is 288px, but it is really 271px because of the scrollbar(width 17px) on Windows.

The 2nd reason is that the emoji width is different. Each emoji box is 12.5% wide of the parent element, but emoji can overflow this width as you can see below.

image

That's why, emoji row can exceed the width(100%) and overflow under the header bar. This is the root cause

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

This is a windows specific issue and I think we need to have a solution for windows platform(Windows specific code may be needed)

  1. Set the emojiHeaderContainer style here

        width: 288;

    Platforms other than Windows has this value already, but Windows has smaller value because of scrollbar. Setting this has no impact on other platforms

  2. Update the EmojiPickerMenuItem component

    • Introduce a new state scale and new member function onLayout

      this.state = {
          scale: getOperatingSystem() === CONST.OS.WINDOWS ? 0 : 1,
      };
      
      this.onLayout = this.onLayout.bind(this);
    onLayout(event) {
        const width = event.nativeEvent.layout.width;
        if (width < 36) {
            this.setState({scale: 1});
        } else {
            this.setState({scale: 36 / width});
        }
    }
        {(this.state.scale) ? (
            <View style={[styles.flexRow, styles.justifyContentCenter, styles.alignItemsCenter]}>
                <Text style={[styles.emojiText, {transform: `scale(${this.state.scale}, 1)`}]}>{this.props.emoji}</Text>
            </View>
        ) : (
            <Text style={[styles.emojiText, styles.hiddenElementOutsideOfWindow]} onLayout={this.onLayout}>{this.props.emoji}</Text>
        )}

Please check the screenshot

Result

What alternative solutions did you explore? (Optional)

allstarsmen commented 10 months ago

Proposal

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

There are 2 issues here:

  1. The width of the emoji row is bigger than the width of the emoji header which leads to the issue in the picture below.

This issue happens

  • Windows 11: Microsoft Edge / Chrome / Opera
  • Ubuntu 22.04.02 LTS: Chrome / Opera

Screenshot 2023-07-21 at 13 49 05

  1. The browser takes space to add the scrollbar in the emoji list which leads to the emojis unaligned with the Category header. This issue happens
    • Windows 11: Microsoft Edge / Chrome / Opera
    • Ubuntu 22.04.02 LTS: Chrome / Opera
    • MacOs Ventura: Safari / Chrome / Firefox / Opera

Screenshot 2023-07-21 at 13 54 41

What is the root cause of that problem?

There are 2 issues here:

  1. The width of the emoji row is bigger than the width of the emoji header which leads to the issue in the picture below.
  2. The browser takes space to add the scrollbar in the emoji list which leads to the emojis unaligned with the Category header.

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

  • To solve issue 1: add {overflow: 'hidden'} to each row of the emoji
  • To solve issue 2: add the calculated width to each row of the emoji

Result

Tested:

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 10 months ago

📣 @allstarsmen! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
allstarsmen commented 10 months ago

Contributor details Your Expensify account email: allstarsmen@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01b99d77c904ab8b0b

melvin-bot[bot] commented 10 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

mallenexpensify commented 10 months ago

@Santhosh-Sellavel , please review the above proposals

Santhosh-Sellavel commented 10 months ago

@Expensify/design Here is the screenshot of the problem

253701733-072b5ddd-0af7-42b4-89d9-2bb89c47628b

What do you think we should do here?

We have a few suggestions

  1. Remove Right Padding - Looks bad without no spacing
  2. Fixed width - Looks bad unaligned
  3. Overflow hide - Emoji will get cut @s-alves10 Can you provide a screen for any case?
  4. Reduce the size of the emoji to 12.4% - This seems to look nice
  5. Do nothing - I don't see any value here
shawnborton commented 10 months ago

Hmm how and when did this start happening though? Is this a regression?

Santhosh-Sellavel commented 10 months ago

This happening on windows, we might had missed this. Not sure about its regression or not.

s-alves10 commented 10 months ago

12.4% can't solve this issue as you can see below.

image

Also this would make the row width less than the header

overflow: hidden

image

unaligned comes from the different size of the emojis. Not from the fixed header width. Fixed header width doesn't affect the align of the emojis. Only fixes this issue(overflow of row)

allstarsmen commented 10 months ago

Proposal

Updated

Santhosh-Sellavel commented 10 months ago

@s-alves10

12.4% can't solve this issue as you can see below.

Thanks for that!

overflow: hidden

image

unaligned comes from the different size of the emojis. Not from the fixed header width. Fixed header width doesn't affect the align of the emojis. Only fixes this issue(overflow of row)

Can we get how it looks, please?

Santhosh-Sellavel commented 10 months ago

@allstarsmen

This is your recording, this makes emojis unaligned with the Category header and looks congested

https://github.com/Expensify/App/assets/85645967/18b9bcb3-91c7-4915-ad44-8ae8a7582bf8

Santhosh-Sellavel commented 10 months ago

So we are waiting for proposals here

s-alves10 commented 10 months ago

image

Above is an example of unaligned(red rectangle). fixed header width means the green rectangle

Santhosh-Sellavel commented 10 months ago

@s-alves10 What's your proposal here? How emojis will look post your changes?

If it looks like the video here, then it's bad, have you attached a wrong video in the result?

s-alves10 commented 10 months ago

As I mentioned earlier, those unaligned emojis(only on Windows) are not from this changes. My solution is to fix this issue.

@Santhosh-Sellavel Do you think we need to fix those unaligned emojis in this issue?

Santhosh-Sellavel commented 10 months ago

those unaligned emojis(only on Windows) are not from this changes

Those alignment also should be fixed here

s-alves10 commented 10 months ago

@Santhosh-Sellavel

Then we should consider the overflowX: 'hidden' solution. That would work though some emojis would be cut off

allstarsmen commented 10 months ago

@s-alves10 How do you get 9 icons in the same row (in the red box)? I have tried many ways but can't get 9 icons in the same row. I only get 8 icons. I tested with the latest Chrome on Windows 11

Screenshot 2023-07-21 at 09 17 46

allstarsmen commented 10 months ago

Proposal

I have updated my proposal again.

melvin-bot[bot] commented 10 months ago

@mallenexpensify @Santhosh-Sellavel this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

Santhosh-Sellavel commented 10 months ago

@allstarsmen I see excessive changes made in the proposal, please submit a proposal in a new comment in similar scenarios. Also please include the video in the comment itself instead of links.

Please avoid posting redundant proposals, @s-alves10 proposed the same changes already, thanks!

allstarsmen commented 10 months ago

Proposal

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

  1. No space should be visible on the right side of the emoji header.
  2. The emojis should be aligned with the Category header.
  3. Some emojis are missing and displaying incorrectly in Windows 10

What is the root cause of that problem?

  1. The total width of emojis in a row is bigger than the width of its parent element. Its parent element and the emoji header have the same width. That leads to some spaces on the right side of the emoji header.
  2. In some browsers (ex: Chrome, and Safari on macOS), the scrollbar takes some space off the emoji list, that's why the width of the emoji list is smaller than it should be and leads to the emoji row and the emoji header not being unaligned.
  3. Windows 10 does not support some emojis that we are using, which leads to some missing and displaying incorrectly.

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

  • In order to solve issue 1 and issue 2, I suggest adding the code below to each row of the emoji list. But only apply the code below when the value of isSmallScreenWidth is FALSE. The value of width is calculated based on the value of EMOJI_PICKER_SIZE.WIDTH in CONST.js and ph4.paddingHorizontal in spacing.js. If the value of EMOJI_PICKER_SIZE.WIDTH and/or ph4.paddingHorizontal are changed the script will auto-re-calculate the value of width.

    {
       overflow: 'hidden',
       width: 288px
    }

    In StyleUtils.js, add the function "getEmojiRowStyle" in the picture below

    Screenshot ![Screenshot 2023-07-28 at 09 08 40](https://github.com/Expensify/App/assets/497313/ca813cc3-c0a4-408f-bd52-520468fd85af)

    In EmojiPickerMenu/index.js, add one line to call the function "getEmojiRowStyle" as in the picture below

    Screenshot ![Screenshot 2023-07-28 at 09 07 46](https://github.com/Expensify/App/assets/497313/580666d2-9d99-4ed5-bb3e-44af378d488d)
  • In order to solve issue 3, I suggest using this font for Windows only.

    1. Download the font and add it to assets/fonts/web
    2. Add the code below to font.css
      @font-face {
          font-family: ExpensifySegoe-UI-Emoji;
          src: url('/fonts/seguiemj.ttf');
      }
    3. In this index.js, add the code below
      if (getOperatingSystem() === CONST.OS.WINDOWS) {
         _.each(fontFamily, (v, k) => fontFamily[k] = 'ExpensifySegoe-UI-Emoji, ' + v)
      }

Result

https://github.com/Expensify/App/assets/497313/9d3f06f3-9b2b-4125-bf9a-38450a688c9a

https://github.com/Expensify/App/assets/497313/778cdef2-90bd-48e1-ad44-6a56481e57a5

https://github.com/Expensify/App/assets/497313/aa3d476d-b642-4b3d-a268-bec0dc67453b

https://github.com/Expensify/App/assets/497313/1de637d5-8a3f-4fb8-bb5c-e73cccc20c06

https://github.com/Expensify/App/assets/497313/52cd4d3b-be51-46ec-8e08-0623d5cfe4c1

https://github.com/Expensify/App/assets/497313/5ef1b980-b92f-409d-b586-5588b67d7ad1

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

samh-nl commented 9 months ago

Bump

Santhosh-Sellavel commented 9 months ago

Will look into this later today, no bounty update needed cc: @mallenexpensify

mallenexpensify commented 9 months ago

@Santhosh-Sellavel please review the above proposals, this is about to be taken internal cuz we're reaching the 4-week mark.

Santhosh-Sellavel commented 9 months ago

@allstarsmen

Your solution reduces the width of the emoji, this makes the picker kind of weirdly placed. That is not something we want.

Emojis category bar & emojis still look unaligned for me on Windows 11

IMG_1376

Reduced size

https://github.com/Expensify/App/assets/85645967/8418c1ce-8010-4c07-b87a-b2f57aa837b4

Santhosh-Sellavel commented 9 months ago

@mallenexpensify We can bump the price up if we need to get more attention. If not let's wait for more proposals thanks!

allstarsmen commented 9 months ago

@Santhosh-Sellavel

sorry, I think you reviewed the wrong solution.

The solution that reduces the width of the emoji is my old solution and it does not solve this bug.

I have updated my solution (https://github.com/Expensify/App/issues/21923#issuecomment-1646376787) to add the "width" and "overlay" to each column of the FlatList. Could you please review again with that solution?

melvin-bot[bot] commented 9 months ago

@mallenexpensify @Santhosh-Sellavel this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

Current Updated
![image](https://github.com/Expensify/App/assets/126839717/63d81009-3686-47c8-afc0-eba063b1fe1d) ![image](https://github.com/Expensify/App/assets/126839717/67b67c3c-2686-4c2d-a822-63594f711646)