Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
isagoico opened this issue Apr 30, 2021 · 44 comments · Fixed by #3561
Closed

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

isagoico opened this issue Apr 30, 2021 · 44 comments · Fixed by #3561
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Apr 30, 2021

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

@isagoico isagoico added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Apr 30, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Apr 30, 2021
@MelvinBot
Copy link

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

@sophiepintoraetz sophiepintoraetz added Weekly KSv2 Improvement Item broken or needs improvement. and removed Daily KSv2 labels May 3, 2021
@sophiepintoraetz sophiepintoraetz removed their assignment May 3, 2021
@isagoico
Copy link
Author

isagoico commented May 9, 2021

Issue reproducible during today's KI retests

1 similar comment
@isagoico
Copy link
Author

Issue reproducible during today's KI retests

@chiragsalian chiragsalian added the External Added to denote the issue can be worked on by a contributor label May 19, 2021
@MelvinBot
Copy link

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

@chiragsalian
Copy link
Contributor

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

@chiragsalian chiragsalian removed the Weekly KSv2 label May 19, 2021
@arielgreen
Copy link
Contributor

Posted.

@pranshuchittora
Copy link
Contributor

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
  • After this, I created a low res android emulator and fired the app in that emulator. Unfortunately wasn't able to repro that.

  • To emulate an even smaller screen size I tried to run the app in split-screen mode (which I personally do a lot).

Voila 🤟🏼

Approach 👨🏼‍💻

File of concern : EmojiPickerMenuItemjs and styles.js

  • The styles are bounded to a width of 12.5% to show 8 items per row
    emojiItem: {
        width: '12.5%',
        height: 40,
        textAlign: 'center',
        borderRadius: 8,
    },
  • 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 🙅🏼‍♂️

@isagoico
Copy link
Author

Issue reproducible during today's KI retests

@arielgreen
Copy link
Contributor

cc @chiragsalian please review proposal above

@chiragsalian
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

isagoico commented May 24, 2021

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

@parasharrajat
Copy link
Member

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

@pranshuchittora
Copy link
Contributor

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

  • Is there any existing utility that implements media query breakpoints and calls the callback provided. If not then we build this utility

@arielgreen
Copy link
Contributor

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

@pranshuchittora
Copy link
Contributor

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

Applied :)

@arielgreen
Copy link
Contributor

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

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@chiragsalian
Copy link
Contributor

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

@pranshuchittora
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

pranshuchittora commented Jun 6, 2021

@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
Copy link
Author

isagoico commented Jun 7, 2021

Issue reproducible today during KI retests

@chiragsalian
Copy link
Contributor

chiragsalian commented Jun 7, 2021

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
Copy link
Contributor

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
Copy link
Contributor

pranshuchittora commented Jun 8, 2021

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
Copy link
Contributor

@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
Copy link
Contributor

chiragsalian commented Jun 11, 2021

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
Copy link
Author

Issue reproducible today during KI retests

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@isagoico
Copy link
Author

Issue reproducible during KI retests.

@isagoico
Copy link
Author

isagoico commented Jul 5, 2021

Issue reproducible during KI retests.

@arielgreen arielgreen changed the title Emojis - Emojis are cut on the sides in emoji picker [Hold for payment 2021-07-06] Emojis - Emojis are cut on the sides in emoji picker Jul 6, 2021
@arielgreen
Copy link
Contributor

Reopening, will close again upon payment.

@arielgreen arielgreen reopened this Jul 6, 2021
@arielgreen arielgreen added Weekly KSv2 and removed Daily KSv2 labels Jul 6, 2021
@arielgreen arielgreen changed the title [Hold for payment 2021-07-06] Emojis - Emojis are cut on the sides in emoji picker [Hold for payment 2021-07-13] Emojis - Emojis are cut on the sides in emoji picker Jul 6, 2021
@arielgreen
Copy link
Contributor

Paid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants