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.18k stars 2.66k forks source link

[Hold for payment 2021-07-13] Emojis - Emojis are cut on the sides in emoji picker #2662

Closed isagoico closed 3 years ago

isagoico 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!


Expected Result:

Emojis are displayed without cropping

Actual Result:

Emojis are inappropriately cropped on the left-hand side

Action Performed:

  1. Open app on an android device
  2. Navigate to a conversation
  3. Open the emoji picker

Workaround:

No need, visual issue.

Platform:

Where is this issue occurring?

Web iOS Android ✔️ Desktop App Mobile Web

Version Number:

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

image

Expensify/Expensify Issue URL:

View all open jobs on Upwork

Job posting on Upwork: https://www.upwork.com/jobs/~015b529edefb069ef8

MelvinBot commented 3 years ago

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

MelvinBot commented 3 years ago

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

isagoico commented 3 years ago

Issue reproducible during today's KI retests

isagoico commented 3 years ago

Issue reproducible during today's KI retests

MelvinBot commented 3 years ago

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

chiragsalian commented 3 years ago

@arielgreen can you post the issue on upwork for someone to suggest a proposal to tackle the problem.

arielgreen commented 3 years ago

Posted.

pranshuchittora commented 3 years ago

Proposal 📄

Emojis are inappropriately cropped on the left-hand side

Actually they are getting cropped on both sides.

Investigation 🕵🏻‍♂️

  • I started by launching the app in my android emulator in which I am not able to repro this issue. Then I carefully looked at the attached screenshot in the issue description from which I am able to infer the following
  • Looks like MI
  • Low res screen

Voila 🤟🏼

Approach 👨🏼‍💻

File of concern : EmojiPickerMenuItemjs and styles.js


- A long term solution is to set that value dynamically based on the device width/height (depends on view mode i.e. Landscape or Portrait)
- Solution might look similar to media queries.

- Another possible solution is to fix the height & width of the icons and let them flex over the container. Pun intended :)

## Best Practices 💃🏼
- No inline styling
- Platform agnostic solution

## Testing Strategy 🧪
- Basic render tests

## Expected Delivery Time 📦 
Approx 2-6 days.

## Previous Experience 🙅🏼‍♂️
- https://github.com/Expensify/Expensify.cash/issues/2811
isagoico commented 3 years ago

Issue reproducible during today's KI retests

arielgreen commented 3 years ago

cc @chiragsalian please review proposal above

chiragsalian commented 3 years ago

Hi @pranshuchittora, thank you for the analysis. You did suggest multiple solutions so I was wondering what solution were you going to go ahead with. For the solution you are going ahead with, can you explain your approach in a little more detail before I can approve it. I personally think the media queries route is fine since it looks like quite a rare device case.

chiragsalian commented 3 years ago

Also @isagoico can you share what android device you are testing on? It would be nice if @pranshuchittora can test against the same device on his emulator as well.

isagoico commented 3 years ago

Sure! My device is Huawei P20 Pro / Android 10.

parasharrajat commented 3 years ago

The best approach would be to show 7 columns for small devices, again using the media query.

pranshuchittora commented 3 years ago

u did suggest multiple solutions so I was wondering what solution were you going to go ahead with. For the solution you are going ahead with, can you explain your approach in a little more detail before I can approve it. I personally think the media queries route is fine since it looks like

Hi, @chiragsalian I am thinking of implementing an approach that consists of media queries to dynamically generate the width.

I will start by figuring out some reasonable threshold like width items per row
320 4
480 5
640 6
840 7
>840 8

These are sample values 👆🏼

Then the width will be calculated by ${100/items_per_row}%

Open Questions

shawnborton commented 3 years ago

@pranshuchittora that feels like it might be a bit overly complex. Keep in mind that 8 columns seems to work fine for most screens that are at least 350px wide. Perhaps we really only need two sizes, one that has 6 columns and one that has 8 columns.

pranshuchittora commented 3 years ago

@pranshuchittora that feels like it might be a bit overly complex. Keep in mind that 8 columns seems to work fine for most screens that are at least 350px wide. Perhaps we really only need two sizes, one that has 6 columns and one that has 8 columns.

Cool then I will handle 2 variations but there’s an edge can of screen resizing. If we fix the width on the mounting of the component then it may break if the user resizes the app.

On iPad I usually use apps in side-by-side mode or in popup mode (iPhone like).

On Phone I use a split-screen mode which significantly changes the dimensions of the app window.

To solve this we can put up an event listener

  onChange = ({ window, screen }) => {
    this.setState({ dimensions: { window, screen } });
  };

  componentDidMount() {
    Dimensions.addEventListener("change", this.onChange);
  }

  componentWillUnmount() {
    Dimensions.removeEventListener("change", this.onChange);
  }
parasharrajat commented 3 years ago

Could this be helpful https://reactnative.dev/docs/0.63/pixelratio#get?

chiragsalian commented 3 years ago

@pranshuchittora did you have a look at @parasharrajat post and is it helpful?

Additionally, I don't quite follow why an eventListener is needed. Wouldn't just,

@media only screen and (max-width: 350px)

suffice for widths 350 or less to set 6 emojis per row? Does that not work in this scenario?

pranshuchittora commented 3 years ago

Hi @chiragsalian, yes I took a look at it but pixel density might not be relevant. Why?

chiragsalian commented 3 years ago

@pranshuchittora

eventListener adds safety and makes sure that UI gets recalculated when app dimension changes.

Do you have an example in mind where media query won't do the same. Like a case you mentioned was this,

I use a split-screen mode which significantly changes the dimensions of the app window.

So what I did was load https://www.w3schools.com/css/tryit.asp?filename=trycss_mediaqueries_ex1 into my iPad and set the different dimensions as 410px, 600px, and default. The background color CSS immediately took into effect when I was in landscape, portrait, or split-screen mode. I guess I'm wondering what use case you are thinking of that media query doesn't solve since it feels like the easier solution.

pranshuchittora commented 3 years ago

@chiragsalian media query will work only for the web. In React Native (considering all platforms) we can't use media query.

Something like this -> https://www.npmjs.com/package/react-native-media-queries

chiragsalian commented 3 years ago

Ahh okay, awesome, alright thank you for explaining. Yeah I'm a fan with the rest of your proposal with the one suggestion that if you are defining size variables somewhere then define them here.

pranshuchittora commented 3 years ago

Ahh okay, awesome, alright thank you for explaining. Yeah I'm a fan with the rest of your proposal with the one suggestion that if you are defining size variables somewhere then define them here.

Sure, will place all the constants in variables.js and will also define proper doc. Is there any specific place for docs? Asking this because there might be some internal documentation 🤔


Thanks a lot for the positive feedback it really means a lot. Looking forward to an awesome collaboration with you all. Good day :)

arielgreen commented 3 years ago

@pranshuchittora Please apply on Upwork so I can send over the offer.

pranshuchittora commented 3 years ago

@pranshuchittora Please apply on Upwork so I can send over the offer.

Applied :)

arielgreen commented 3 years ago

Excellent! Thanks for the quick turnaround. Just sent over an offer, @pranshuchittora.

isagoico commented 3 years ago

Issue reproducible today during KI retests

chiragsalian commented 3 years ago

@pranshuchittora any update on the PR to resolve this GH issue?

pranshuchittora commented 3 years ago

Hi @chiragsalian I spent a significant amount of time trying various approaches. To make screen resizing changes the items per row.

  1. Dynamic numColumns. The idea is to change the number of columns dynamically on the FlatList. Unfortunately we can't change it dynamically or on run time. https://stackoverflow.com/questions/44291781/dynamically-changing-number-of-columns-in-react-native-flat-list

A workaround is to re-render the component which is expensive.

  1. Dynamic Width of each emoji. This overrides the fixed width of 12.5%. The drawback is that it is calculated on all the items (100s of emojis.)

Another blocker is that there is a new API for listening for dimensions https://reactnative.dev/docs/usewindowdimensions. This is a react hook and can't be used in class components.

Need your thoughts on all these 🤔

chiragsalian commented 3 years ago

Hmm is it not possible to use WithWindowDimensions more specifically onDimensionChange to help you out. Similar to how we do it here with ReportActionCompose to detect a change in dimensions to update the compose menu accordingly?

pranshuchittora commented 3 years ago

@chiragsalian there's some bug in the implementation of flat list.

What I did Just change the number of columns.

Screenshot from 2021-06-06 21-54-11

This is probably due to the headers being used in flatlist.

We should use Setcion List instead .https://reactnative.dev/docs/sectionlist

I am able to make this responsive thing work but this numColumns is breaking UI like this.


IMO, Section List is the right component for this implementation. And we can even get rid of iffy code in renderItem.

isagoico commented 3 years ago

Issue reproducible today during KI retests

chiragsalian commented 3 years ago

Hmm so i believe @stitesExpensify originally wrote the emoji flatList. Stites could you share your thoughts on how we can dynamically change the number of columns depending on the width of the mobile device? Are you aware of a simple way to make this change using flatList or do you feel like we should change this to sectionList?

I do think that sectionList than flatList sounds like a good change since it makes sense that the emoji's are pretty much sections. But that change also sounds like a slightly more involved change. I could be wrong but I feel like if we were to change this to sectionList then the data in our emoji.js would change quite a bit as well.

stitesExpensify commented 3 years ago

how we can dynamically change the number of columns depending on the width of the mobile device

We can't, at least not easily. I think that just changing the size of the emojis would be a better idea personally.

Dynamic Width of each emoji. This overrides the fixed width of 12.5%. The drawback is that it is calculated on all the items (100s of emojis.)

I don't think we need to dynamically change the width of each emoji, we can probably just make all emojis smaller right?

do you feel like we should change this to sectionList?

I had initially attempted to use sectionList because it does seem to fit our use case well, but there are features missing in the RN implementation that cause it to not work. Specifically that it doesn't have column support (you can only have one item per row), and the only things I could find online to fix it were hacks.

pranshuchittora commented 3 years ago

I had initially attempted to use sectionList because it does seem to fit our use case well, but there are features missing in the RN implementation that cause it to not work. Specifically that it doesn't have column support (you can only have one item per row), and the only things I could find online to fix it were hacks.

What I use to do is render flat list inside section. Something like this

<SectionList
          sections={vendors}
          renderSectionHeader={({ section }) => (
            <Text> {section.category}</Text>
          )}
          renderItem={({ section }) => (
            <FlatList
              data={section.data}
              horizontal={true}
              renderItem={({ item }) => <Text>{item.name}</Text>}
            />
          )}
        />

Will try with the width

pranshuchittora commented 3 years ago

@chiragsalian making emojis smaller worked. But they look very small on the bigger viewport (like the browser).

Screenshot from 2021-06-10 00-37-19

Should I implement the change in size proportional to the change in width?

chiragsalian commented 3 years ago

Should I implement the change in size proportional to the change in width?

Sure but lets just keep two sizes i.e., the normal one we use now, and for a smaller width screen we change the emoji size to something smaller. Would that work?

isagoico commented 3 years ago

Issue reproducible today during KI retests

isagoico commented 3 years ago
Issue reproducible today during KI retests

isagoico commented 3 years ago

Issue reproducible during KI retests.

isagoico commented 3 years ago

Issue reproducible during KI retests.

arielgreen commented 3 years ago

Reopening, will close again upon payment.

arielgreen commented 3 years ago

Paid.