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

Edit box - Text scroller appears if the edited text contains a emoji #2850

Closed
isagoico opened this issue May 13, 2021 · 24 comments · Fixed by #3237
Closed

Edit box - Text scroller appears if the edited text contains a emoji #2850

isagoico opened this issue May 13, 2021 · 24 comments · Fixed by #3237
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@isagoico
Copy link

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:

Edit comment should not have a scrollbar if there's a emoji in a single line text.

Actual Result:

Scrollbar appears when adding a emoji to the edited text.

Action Performed:

  1. Go to staging.expensify.cash
  2. Log in with any account
  3. Navigate to a conversation
  4. Send a single emoji
  5. Edit the message

Workaround:

Unknown

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.43-0

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

Notes/Photos/Videos: Any additional supporting documentation
Similar to #2599 but it's affecting only the edit comment box.

image

Expensify/Expensify Issue URL:

View all open jobs on Upwork

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

Triggered auto assignment to @RachCHopkins (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 May 13, 2021
@RachCHopkins
Copy link
Contributor

Ummm. I @isagoico have no way to send a message on staging. There is no message box whatsoever:

image

@isagoico
Copy link
Author

Yep it's a weird issue, sometimes reloading the app or refreshing the page solves it #2804

@RachCHopkins
Copy link
Contributor

Ok, no scrollbar for me on MacOS / Chrome

image

What OS/Browser combo are you using @isagoico ?

@isagoico
Copy link
Author

Issue reproducible during today's KI retests

@RachCHopkins
Copy link
Contributor

What OS/Browser combo are you using @isagoico ?

@jasperhuangg
Copy link
Contributor

This is definitely an issue with Windows if it isn't reproducible on Mac.

@RachCHopkins
Copy link
Contributor

Ok, based on that I'll add the label!

@MelvinBot
Copy link

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

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@MelvinBot
Copy link

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

@trjExpensify
Copy link
Contributor

Seems like the engineering assignment was removed accidentally, pulling the trigger again for review.

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

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

@AndrewGable AndrewGable removed their assignment May 26, 2021
@JmillsExpensify
Copy link

Jumping in to post on Upwork now.

@JmillsExpensify
Copy link

If you are interested in working on this issue, then post directly here in GH with a proposal with the technical explanation of the changes you will make. Somebody from Expensify will review your proposal in GitHub, and if your proposal is accepted, you will be hired in Upwork for the job. Thank you!

@JmillsExpensify
Copy link

Upwork job posting for reference: https://www.upwork.com/jobs/~01db49fd6e066d5af8

@MelvinBot
Copy link

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

@aliabbasmalik8
Copy link
Contributor

PROPOSAL
We can achieve this by updating the style here
https://github.com/Expensify/Expensify.cash/blob/b4e07c60df0c1edf7f48c320c7ec36609648a3d6/src/pages/home/report/ReportActionItemMessageEdit.js#L99

<View style={[styles.chatItemComposeBox, styles.flexRow, styles.chatItemComposeBoxColor]}>
    <TextInputFocusable
        multiline
        onChangeText={this.updateDraft} // Debounced saveDraftComment
        onKeyPress={this.triggerSaveOrCancel}
        defaultValue={this.props.draftMessage}
        maxLines={16} // This is the same that slack has
        style={[styles.textInputCompose, styles.flex4]}
        onFocus={() => {
            scrollToIndex({animated: true, index: this.props.index}, true);
            toggleReportActionComposeView(false);
        }}
        autoFocus
    />
</View>

NOTE: above used styling object already used in-app for new comment <TextInput>.

@dklymenk
Copy link
Contributor

dklymenk commented May 28, 2021

Managed to reproduce the issue on Windows (no issue on Linux). It looks like emojis have higher line-height on windows.

linux, no issue, emoji is inside textarea:
DeepinScreenshot_select-area_20210528135554

windows, emoji is a few pixels higher than textarea:
image

The line-height for textarea is set to normal by default, which should be fine but causes this weird behavior.

Setting textarea line-height to any of these fixes the issue:

  • inherit
  • unset
  • any number like 1, 1.2. Even If I put 0 there is no scroll bar

My proposal is to add lineHeight: 'unset', here:

https://github.com/Expensify/Expensify.cash/blob/main/src/styles/styles.js#L319-L333

It's going to look like this on windows:
image

and like this on linux(I would assume mac too):
DeepinScreenshot_select-area_20210528142636

Won't affect native because native implementation of TextInputFocusable doesn't use that style.

EDIT: Will need to check native because that particular style is used across the app for many other TextInputs. Solution by @aliabbasmalik8 might be better, if he means adding new lineHeight style to component itself and not to styles.textInput as I have suggested.

@chiragsalian
Copy link
Contributor

Nice thank you for the proposals and yes I'm leaning towards the one by @aliabbasmalik8 since it will just affect this component.

@aliabbasmalik8 feel free to create a PR to solve this.

@chiragsalian
Copy link
Contributor

Oh also @aliabbasmalik8, i dont see your proposal for this issue in upwork. Please post your proposal there too so that we can hire and pay you for the work 🙂

@chiragsalian
Copy link
Contributor

@JmillsExpensify, feel free to accept Ali's proposal in Upwork whenever you get the chance.

@JmillsExpensify
Copy link

Jumping in now!

@isagoico
Copy link
Author

Issue reproducible today during KI retests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.