fix: removed line height from all inputs #15804

@techievivek @aimane-chnaif One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

aimane-chnaif · 2023-03-09T23:48:44Z

@allroundexperts this should be applied to composer as well

src/components/TextInput/BaseTextInput.js

allroundexperts · 2023-03-10T09:02:35Z

@aimane-chnaif Updated.

aimane-chnaif · 2023-03-10T09:11:50Z

src/styles/styles.js

@@ -1400,6 +1401,9 @@ const styles = {

    // Be extremely careful when editing the compose styles, as it is easy to introduce regressions.
    // Make sure you run the following tests against any changes: #12669


@allroundexperts please test #12669 as comment says.
I don't think it's related but just to be safe.

@aimane-chnaif Android and iOS attached.

Screen.Recording.2023-03-10.at.2.16.05.PM.mov

Screen.Recording.2023-03-10.at.2.18.18.PM.mov

src/components/TextInput/BaseTextInput.js

aimane-chnaif · 2023-03-10T11:40:30Z

Here's comparison per each platform:

diff.mov

cc: @shawnborton

aimane-chnaif · 2023-03-10T15:36:12Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mchrome.mov

Mobile Web - Safari

msafari.mp4

Desktop

desktop.mov

iOS

ios.mp4

Android

android.mov

aimane-chnaif

@shawnborton @techievivek please check videos, especially in #15804 (comment).
I think no problem with changes and approving but will defer to you.

shawnborton · 2023-03-10T15:53:25Z

Hmm I'm gonna be honest, I don't love that now the composer box has a tighter line height - now the text you type doesn't match the same line height styling it would have when the text is sent and turned into a message. Is there a way to only do this for our text inputs and not the composer itself?

aimane-chnaif · 2023-03-10T16:01:19Z

Is there a way to only do this for our text inputs and not the composer itself?

yes, we might not want to touch composer but unfortunately cursor flicker also happens in android on composer though barely noticeable to users.
Cursor height mismatch between first line and other lines on Safari, mSafari also happens but I think this one can be ignored if not really needed to fix.

aimane-chnaif · 2023-03-10T16:09:45Z

@shawnborton so which options do you want this fixed by?

About composer:

keep line height on all platforms (find better solution to fix glitch and if no solution, do nothing)
remove line height on android only and keep on all other platforms (suggested)
remove line height on all platforms (current PR)

Same options for other multiline inputs (close account, invite members)

allroundexperts · 2023-03-10T16:37:54Z

@shawnborton @aimane-chnaif Just FYI, On android for the composer, there is a change in size of composer when you enter the first character of second line. This is also caused by the line height being set. Removing line height fixes that jump as well.

Screen.Recording.2023-03-10.at.9.31.28.PM.mov

shawnborton · 2023-03-10T20:28:26Z

Interesting, got it. Okay, well I guess it makes sense to move forward with this and see how it feels then.

aimane-chnaif · 2023-03-10T22:25:12Z

@allroundexperts can you also test #15864?

allroundexperts · 2023-03-10T22:57:52Z

@allroundexperts can you also test #15864?

iOS:
With LH:

Without LH:

aimane-chnaif · 2023-03-10T22:59:29Z

@allroundexperts can you also test #15864?

iOS: With LH:

Without LH:

oops. too close. can you also test and share android diff?

shawnborton · 2023-03-10T23:06:07Z

Hmmm the lines on iOS feel way too close together now. I'm starting to think that maybe this is not the best solution just to fix a cursor glitch in Android. Are we sure there isn't a better solution that can solve the Android issue without causing all of these other design inconsistencies?

allroundexperts · 2023-03-10T23:07:58Z

@allroundexperts can you also test #15864?

iOS: With LH:
Without LH:

oops. too close. can you also test and share android diff?
@aimane-chnaif

shawnborton · 2023-03-10T23:09:15Z

Can you share before/after on iOS with two lines of text (no emojis) please?

allroundexperts · 2023-03-10T23:10:25Z

Hmmm the lines on iOS feel way too close together now. I'm starting to think that maybe this is not the best solution just to fix a cursor glitch in Android. Are we sure there isn't a better solution that can solve the Android issue without causing all of these other design inconsistencies?

@shawnborton We probably should not apply this to composer. There isn't any noticeable flickering in the composer anyways. For Close account multiline input, it looks fine because there are no emojis there.

allroundexperts · 2023-03-10T23:11:46Z

Can you share before/after on iOS with two lines of text (no emojis) please?

aimane-chnaif · 2023-03-10T23:12:52Z

There isn't any noticeable flickering in the composer anyways

can you find root cause why barely noticeable in composer than other multiline inputs?
I think those have same lineHeight, fontFamily and fontSize right now.

it looks fine because there are no emojis there.

You still can input emoji though.

Hmmm the lines on iOS feel way too close together now. I'm starting to think that maybe this is not the best solution just to fix a cursor glitch in Android. Are we sure there isn't a better solution that can solve the Android issue without causing all of these other design inconsistencies?

I agree fixing cursor glitch is not more important than too close between lines.

allroundexperts · 2023-03-10T23:16:26Z

There isn't any noticeable flickering in the composer anyways

can you find root cause why barely noticeable in composer than other multiline inputs? I think those have same lineHeight, fontFamily and fontSize right now.

it looks fine because there are no emojis there.

You still can input emoji though.

Hmmm the lines on iOS feel way too close together now. I'm starting to think that maybe this is not the best solution just to fix a cursor glitch in Android. Are we sure there isn't a better solution that can solve the Android issue without causing all of these other design inconsistencies?

I agree fixing cursor glitch is not more important than too close between lines.

It's because initially composer is a single line input. It only becomes multiline once you go over to the next line. With other inputs, they are multiline since the beginning.

aimane-chnaif · 2023-03-10T23:18:10Z

Seeing #15864, I see that current lineHeight set for all texts are not big enough for emojis look good in multiline. (both input view and normal text view)

aimane-chnaif · 2023-03-10T23:27:55Z

@shawnborton so which options do you want this fixed by?

About composer:

keep line height on all platforms (find better solution to fix glitch and if no solution, do nothing)

remove line height on android only and keep on all other platforms (suggested)

remove line height on all platforms (current PR)

Same options for other multiline inputs (close account, invite members)

So now I'd like to get suggestions from both of you @shawnborton @techievivek

shawnborton · 2023-03-10T23:29:32Z

Which one is before or after in this comment?

aimane-chnaif · 2023-03-10T23:47:44Z

Which one is before or after in this comment?

I believe small one is after fix but this depends device font size.
If device font size is very small, after-fix is smaller.
If device font size is very big, after-fix is larger.

allroundexperts · 2023-03-11T10:11:58Z

Which one is before or after in this comment?

@shawnborton Small one is after the fix.

aimane-chnaif · 2023-03-13T15:12:53Z

How do we proceed here? @shawnborton @techievivek

shawnborton · 2023-03-13T20:17:32Z

I kind of think we should go back to the drawing board here and try to find a better solution honestly. This one is starting to feel a bit hacky to me. Thoughts on that?

aimane-chnaif · 2023-03-13T20:26:22Z

I kind of think we should go back to the drawing board here and try to find a better solution honestly. This one is starting to feel a bit hacky to me. Thoughts on that?

I agree. @techievivek if you +1, can we close this PR, go back to the issue and find better solution?
Sorry to @allroundexperts

allroundexperts · 2023-03-13T20:30:00Z

@aimane-chnaif Thanks for letting me know. Do I get any compensation for this? Previously, I got paid if the PR was created but not merged due to this sort of issue. Ref

aimane-chnaif · 2023-03-13T20:32:05Z

@aimane-chnaif Thanks for letting me know. Do I get any compensation for this? Previously, I got paid if the PR was created but not merged due to this sort of issue. Ref

I experienced this first time, so I am not sure. I will defer to @techievivek and @zanyrenney

techievivek · 2023-03-17T08:45:18Z

Hmmm... it looks like getting rid of line height is not a good choice over here. @allroundexperts, do you think you can explore some other solution here, or should we close this PR and look for a better fix?

allroundexperts · 2023-03-17T08:48:08Z

Hmmm... it looks like getting rid of line height is not a good choice over here. @allroundexperts, do you think you can explore some other solution here, or should we close this PR and look for a better fix?

@techievivek The only other solution I can think of is fixing this on the native side. I'm not sure if I'll be able to do it since I don't have good knowledge of the native code. I'm still not sure why we can't just apply this to the close account text input on android only?

techievivek · 2023-03-21T13:47:43Z

@aimane-chnaif @shawnborton How about fixing this only for Android?

@allroundexperts has shared the screen recordings for Android.
With line-height:
https://user-images.githubusercontent.com/30054992/224184503-cd146bb8-91b4-45a7-b360-ba090d28240b.mp4

Without line-height:
https://user-images.githubusercontent.com/30054992/224184511-3baa391f-ddf0-4282-b3cd-cc6854918190.mp4

In case we don't feel confident with the above solution then I can reach out to Callstack developers and see if we can get anyone to work on this issue. Let me know what you guys think.

aimane-chnaif · 2023-03-21T14:04:56Z

I think this is related to facebook/react-native#35951
App issue: #14367
I am not sure this issue will be fixed after enabling new architecture.
But yes, we can reach out to Callstack if they have solution in native way without removing lineHeight.

techievivek · 2023-03-21T17:51:12Z

@aimane-chnaif Good catch. It looks like this should get fixed after enabling the new architecture.

techievivek · 2023-04-13T06:27:35Z

Closing this PR as the callstack developers will be handling the linked issue now.

fix: removed line height from all inputs

987197a

allroundexperts marked this pull request as ready for review March 9, 2023 23:37

allroundexperts requested a review from a team as a code owner March 9, 2023 23:37

melvin-bot bot requested review from aimane-chnaif and techievivek and removed request for a team March 9, 2023 23:37

allroundexperts mentioned this pull request Mar 9, 2023

[$2000] Flickering is seen when entering first character #14799

Closed

1 task

aimane-chnaif reviewed Mar 9, 2023

View reviewed changes

src/components/TextInput/BaseTextInput.js Outdated Show resolved Hide resolved

fix: removed composer height

fab71d5

allroundexperts force-pushed the fix-14799 branch from 977feeb to fab71d5 Compare March 10, 2023 09:02

allroundexperts requested review from aimane-chnaif and removed request for techievivek March 10, 2023 09:02

aimane-chnaif reviewed Mar 10, 2023

View reviewed changes

allroundexperts requested a review from aimane-chnaif March 10, 2023 09:21

aimane-chnaif reviewed Mar 10, 2023

View reviewed changes

src/components/TextInput/BaseTextInput.js Show resolved Hide resolved

fix: updated comment for removing line height from text input

78c6732

allroundexperts force-pushed the fix-14799 branch from 3e41b89 to 78c6732 Compare March 10, 2023 12:11

techievivek self-requested a review March 10, 2023 12:21

aimane-chnaif approved these changes Mar 10, 2023

View reviewed changes

techievivek closed this Apr 13, 2023

		@@ -1400,6 +1401,9 @@ const styles = {

		// Be extremely careful when editing the compose styles, as it is easy to introduce regressions.
		// Make sure you run the following tests against any changes: #12669

fix: removed line height from all inputs #15804

fix: removed line height from all inputs #15804

Conversation

allroundexperts commented Mar 9, 2023

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Mar 9, 2023

MelvinBot commented Mar 9, 2023

aimane-chnaif commented Mar 9, 2023

allroundexperts commented Mar 10, 2023

aimane-chnaif Mar 10, 2023

Choose a reason for hiding this comment

allroundexperts Mar 10, 2023

Choose a reason for hiding this comment

aimane-chnaif commented Mar 10, 2023

aimane-chnaif commented Mar 10, 2023

Reviewer Checklist

Screenshots/Videos

aimane-chnaif left a comment

Choose a reason for hiding this comment

shawnborton commented Mar 10, 2023

aimane-chnaif commented Mar 10, 2023

aimane-chnaif commented Mar 10, 2023 • edited Loading

allroundexperts commented Mar 10, 2023

shawnborton commented Mar 10, 2023

aimane-chnaif commented Mar 10, 2023

allroundexperts commented Mar 10, 2023 • edited Loading

aimane-chnaif commented Mar 10, 2023

shawnborton commented Mar 10, 2023

allroundexperts commented Mar 10, 2023

shawnborton commented Mar 10, 2023

allroundexperts commented Mar 10, 2023

allroundexperts commented Mar 10, 2023

aimane-chnaif commented Mar 10, 2023

allroundexperts commented Mar 10, 2023

aimane-chnaif commented Mar 10, 2023

aimane-chnaif commented Mar 10, 2023

shawnborton commented Mar 10, 2023

aimane-chnaif commented Mar 10, 2023

allroundexperts commented Mar 11, 2023

aimane-chnaif commented Mar 13, 2023

shawnborton commented Mar 13, 2023

aimane-chnaif commented Mar 13, 2023

allroundexperts commented Mar 13, 2023

aimane-chnaif commented Mar 13, 2023

techievivek commented Mar 17, 2023

allroundexperts commented Mar 17, 2023

techievivek commented Mar 21, 2023

aimane-chnaif commented Mar 21, 2023

techievivek commented Mar 21, 2023

techievivek commented Apr 13, 2023

aimane-chnaif commented Mar 10, 2023 •

edited

Loading

allroundexperts commented Mar 10, 2023 •

edited

Loading