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

Set lineHeight for the report action compose box to 20px #2511

Merged
merged 3 commits into from
Apr 28, 2021

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Apr 21, 2021

Details

For whatever reason, emojis have a line height of 25px, in contrast to the line-height of 20px that plain text has. This means that the check happening here thinks that we need two rows to display the emoji when we only actually need 1.

Fixed Issues

Fixes #2490

Tests

Typed emojis into the report action compose box and verified that they displayed in the correct number of rows.

QA Steps

  1. Navigate to any chat
  2. Type emojis in the report action compose box. Verify that they display in the correct number of rows, aren't cutoff, and don't have any extra vertical space.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-04-21 at 4 52 55 PM

Mobile Web

simulator_screenshot_8D6C4E11-CB1D-401B-999C-B43F21E8B817

Desktop

Screen Shot 2021-04-21 at 4 56 26 PM

iOS

simulator_screenshot_D18875E9-981E-475D-8F2C-A53C99F5F223

Android

Screenshot_1618995607

@jasperhuangg jasperhuangg self-assigned this Apr 21, 2021
@jasperhuangg jasperhuangg requested a review from a team April 21, 2021 11:05
@jasperhuangg jasperhuangg marked this pull request as ready for review April 21, 2021 11:05
@MelvinBot MelvinBot requested review from NikkiWines and removed request for a team April 21, 2021 11:05
@shawnborton shawnborton self-requested a review April 21, 2021 14:27
@shawnborton
Copy link
Contributor

Hey Jasper - I don't think this is the correct approach for this. Can you show me an example of where we have a large paragraph of text? With this quick fix, we know compromise the text style and we'll have a mismatch between how text appears in the compose box versus how text appears as chat messages.

I know @stitesExpensify has been working on emoji-related items, and I think with this particular item we should address the emoji line height when we address making them larger as well. I think ideally the emoji objects get wrapped in something that specifically has a lineHeight of 20 so that it balances out with the rest of the text.

@jasperhuangg
Copy link
Contributor Author

jasperhuangg commented Apr 22, 2021

@shawnborton Looks like you're absolutely correct that there is a very subtle mismatch between the comments and the compose box, thanks for catching this! Do you mind linking the issue that @stitesExpensify is working on?

Screen Shot 2021-04-22 at 10 12 35 AM

I think ideally the emoji objects get wrapped in something that specifically has a lineHeight of 20 so that it balances out with the rest of the text.

Although I agree this is the correct way to move forward with things, one problem is that our current implementation of the report action compose box uses a textarea internally. This means that we won't be able to render JSX inside of it. One option is to change up the implementation of the report action compose box to use a content-editable div, which functions the same as a textarea. The problem is, these aren't supported natively in React Native (only on React web), meaning we'll have to display one inside of a WebView.

Personally, I think this is a valuable thing to spend time on in case we want to implement things like real-time markdown rendering and other special inline elements that wouldn't otherwise be possible with a plain textarea. What are your thoughts?

@stitesExpensify
Copy link
Contributor

So this is something I've been looking into for the comments themselves, but not inside of the compose bar. I agree with you that it's definitely something worth spending time on and investigating given that we will probably want a bunch of live markdown in the future.

That said I don't think that using content-editable div is the way to go, we will definitely want a natively supported React Native component

@jasperhuangg
Copy link
Contributor Author

That said I don't think that using content-editable div is the way to go, we will definitely want a natively supported React Native component

After doing a good amount of research it doesn't seem like there are any React Native TextInputs (and any React Native community libraries) that support rendering JSX and HTML inside of them. Besides using the WebView, we'd have to develop our own content editable div natively, which poses a number of challenges but could potentially be cool to open source (since one doesn't seem to exist). What are your thoughts, and would you potentially be down to work on something like this in the near future?

@stitesExpensify

@NikkiWines
Copy link
Contributor

Looks like there's still some discussion going on here - I'm gonna be OOO for some of next week so gonna let @stitesExpensify take over reviewing this. If it's not merged when I'm back I'm happy to review!

@NikkiWines NikkiWines requested review from stitesExpensify and removed request for NikkiWines April 23, 2021 19:20
@jasperhuangg jasperhuangg changed the title Set lineHeight for the report action compose box to 25px [HOLD] Set lineHeight for the report action compose box to 25px Apr 26, 2021
@jasperhuangg jasperhuangg requested a review from a team as a code owner April 27, 2021 01:09
@MelvinBot MelvinBot requested review from TomatoToaster and removed request for a team April 27, 2021 01:09
@jasperhuangg jasperhuangg changed the title [HOLD] Set lineHeight for the report action compose box to 25px Set lineHeight for the report action compose box to 20px Apr 27, 2021
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll also want to apply this lineHeight style in here as well 🙂 ... That way the issue will be fixed both in the compose box and in the report history.

@jasperhuangg
Copy link
Contributor Author

@roryabraham Good catch! I just checked the link you sent me and it seems like lineHeight: 20 is already being applied there. If it looks good to you then I should be ready for another review, thanks!

@roryabraham roryabraham merged commit 13e3874 into main Apr 28, 2021
@roryabraham roryabraham deleted the jasper-fixEmojiLineHeight branch April 28, 2021 20:44
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.33-1🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented May 8, 2021

🚀 Deployed to production in version: 1.0.39-5🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emoji / Compose box - Compose box content shifts up when an emoji is added.
7 participants