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.33k stars 2.76k forks source link

[$250] Android-Chat-The top of emojis are cut off on applying quote markdown #47899

Open IuliiaHerets opened 3 weeks ago

IuliiaHerets commented 3 weeks 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!


Version Number: 9.0.24 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4884616 Issue reported by: Applause Internal Team

Action Performed:

  1. Laugh app
  2. Tap on a chat
  3. Enter > πŸ˜πŸ˜„πŸ˜ƒπŸ˜‘πŸ˜‘
  4. Send the message

Expected Result:

The top of emojis must not be cut off on applying quote markdown.

Actual Result:

The top of emojis are cut off on applying quote markdown.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/3dc2e163-b5a8-499f-b433-04421ae787e7

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01550e0f7b3eac50c6
  • Upwork Job ID: 1829238843117327522
  • Last Price Increase: 2024-08-29
  • Automatic offers:
    • hoangzinh | Reviewer | 103831336
Issue OwnerCurrent Issue Owner: @hoangzinh
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 3 weeks ago

@strepanier03 FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

melvin-bot[bot] commented 2 weeks ago

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

daledah commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-08-28 10:22:02 UTC.

Proposal

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

The top of emojis are cut off on applying quote markdown.

What is the root cause of that problem?

The emoji's lineHeight for comments containing only emojis are 35:

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/pages/home/report/comment/TextCommentFragment.tsx#L59

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/components/HTMLEngineProvider/HTMLRenderers/EmojiRenderer.tsx#L8

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/styles/index.ts#L1704-L1707

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/styles/variables.ts#L52

While blockquote uses the default lineHeight for HTML renderers, which is smaller than 35:

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.tsx#L96

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/styles/index.ts#L235-L240

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

This issue already happened with EditedRenderer. The solution was to denote these contains-only-emoji edited tags as islarge and use lineHeight: 35 specifically for them:

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/pages/home/report/comment/TextCommentFragment.tsx#L56

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/components/HTMLEngineProvider/HTMLRenderers/EditedRenderer.tsx#L15

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/components/HTMLEngineProvider/HTMLRenderers/EditedRenderer.tsx#L17

So in this case:

  1. Add islarge attribute to contains-only-emoji blockquote following what we've done with emojis:

https://github.com/Expensify/App/blob/f15a1e453b5d596662a581b3c2afba1fcc502705/src/pages/home/report/comment/TextCommentFragment.tsx#L59

let htmlContent = htmlWithDeletedTag;
if (containsOnlyEmojis) {
    htmlContent = Str.replaceAll(htmlContent, '<emoji>', '<emoji islarge>');
    htmlContent = Str.replaceAll(htmlContent, '<blockquote>', '<blockquote islarge>');
}
  1. Now let's customize blockquote's style based on islarge attribute above. In the customHTMLElementModels here, define blockquote custom renderer and customize its style using getMixedUAStyles:
blockquote: HTMLElementModel.fromCustomModel({
    tagName: 'blockquote',
    contentModel: HTMLContentModel.block,
    getMixedUAStyles: (tnode) => {
        if (tnode.attributes.islarge === undefined) {
            return;
        }
        return styles.onlyEmojisTextLineHeight;
    },
})
  1. As this problem only occurs on Android, we might need to define platform-specific onlyEmojisBlockquoteLineHeight styles.
melvin-bot[bot] commented 2 weeks ago

@strepanier03 Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 2 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~01550e0f7b3eac50c6

melvin-bot[bot] commented 2 weeks ago

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

strepanier03 commented 2 weeks ago

Easy to reproduce with any combo of emoji.

hoangzinh commented 2 weeks ago

Thanks for proposal @daledah. Can you elaborate on why the issue is only reproducible on Android?

melvin-bot[bot] commented 1 week ago

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

hoangzinh commented 1 week ago

Hi @daledah. When you have time, can you check my comment here? Thank you

daledah commented 1 week ago

@hoangzinh I think it's a problem with react-native itself. You can check out the demo here: https://snack.expo.dev/b1h4FVhn3ZkBsS6XVQJg6.

hoangzinh commented 1 week ago

Thanks for updates @daledah. I'm thinking about the expectation in this case. When users type emojis with blockquote, should we display emojis in a large style or a normal style?

hoangzinh commented 1 week ago

@Expensify/design Do you have any advice for us in this case? The question is: When users type emojis with blockquote, should we display emojis in a large style or a normal style? Thank you.

Large style Normal style
Screenshot 2024-09-05 at 19 05 50 Screenshot 2024-09-05 at 19 05 34
shawnborton commented 1 week ago

I think we'd follow the same styles that regular messages have. So whatever we do for normal emoji-only messages, we'd do here for blockquotes as well.

hoangzinh commented 1 week ago

Thanks @shawnborton

hoangzinh commented 1 week ago

@daledah's proposal looks good to me

Link to proposal https://github.com/Expensify/App/issues/47899#issuecomment-2314910523

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @aldo-expensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 1 week ago

πŸ“£ @hoangzinh πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 week ago

πŸ“£ @daledah You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 week ago

@hoangzinh @strepanier03 @aldo-expensify @daledah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

daledah commented 1 week ago

@hoangzinh PR is ready.