Resolved the Composer text vertical alignment

[liyamahendra]

Details

Resolved the vertical alignment of text inside the Composer

Fixed Issues

$ #10202

Tests

Test single line padding

1. Login in iOS app
2. Go to any chat
3. Type message in composer and notice the send icon and text written is aligned more to bottom rather than centre vertically
4. Text should be proper aligned to vertically centre

Test multi line padding

1. Login in iOS app
2. Go to any chat
3. Type multi-line message in composer and notice the send icon and text written is aligned more to bottom rather than centre vertically
4. Text should be proper aligned to vertically centre

• Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• 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 */
  • Any functional components have the displayName property
  • 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 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.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

• 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 verified tests pass on all platforms & I tested again on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 */
  • Any functional components have the displayName property
  • 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 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.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

QA Steps

Test single line padding

1. Login in iOS app
2. Go to any chat
3. Type message in composer and notice the send icon and text written is aligned more to bottom rather than centre vertically
4. Text should be proper aligned to vertically centre

Test multi line padding

1. Login in iOS app
2. Go to any chat
3. Type multi-line message in composer and notice the send icon and text written is aligned more to bottom rather than centre vertically
4. Text should be proper aligned to vertically centre

• Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Screencasts

Web

browser.mp4

Mobile Web

mWeb.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

Android.mp4

[Julesssss]

Thanks for raising the PR. Could you add two tests please to the issue description for QA and the reviewers to follow? It can be much simpler for your tests, but this is an example. Thanks!

• Single input padding should be equal
• Multiline input padding should be equal

[liyamahendra]

@Julesssss I'm having a little difficulty with the test. Here is what I've got locally so far:

1. A new file tests/unit/ComposerPaddingTest.js with the below content:

import React from 'react';
import 'react-native';
 import {cleanup, fireEvent, render} from '@testing-library/react-native';
 import {ReportActionCompose} from '../../src/pages/home/report/ReportActionCompose';

 jest.mock('../../src/components/withDrawerState', () => () => ({
    useDrawerStatus: jest.fn(),
}));

jest.mock('../../src/libs/BootSplash', () => () => ({
    getVisibilityStatus: jest.fn(),
}));

jest.mock('@react-native-firebase/crashlytics', () => () => ({
    log: jest.fn(),
    recordError: jest.fn(),
}));

jest.mock('../../src/components/withWindowDimensions', () => () => ({
    withWindowDimensions: jest.fn(),
}));

jest.mock('../../src/pages/home/report/ReportActionCompose', () => () => ({
    betas: jest.fn(),
    onSubmit: jest.fn(),
    reportID: jest.fn(),
    isDrawerOpen: jest.fn(),
    isSmallScreenWidth: jest.fn(),
    isFocused: jest.fn(),
    isComposerFullSize: jest.fn(),
    translate: jest.fn(),
    numberFormat: jest.fn(),
    timestampToRelative: jest.fn(),
    timestampToDateTime: jest.fn(),
    toLocalPhone: jest.fn(),
    fromLocalPhone: jest.fn(),
    fromLocaleDigit: jest.fn(),
    toLocaleDigit: jest.fn(),
}));


 test('Empty composer - Validate top & bottom padding', () => {
    const { findByPlaceholderText } = render(<ReportActionCompose />);
    const input = findByPlaceholderText('Write something...');
    console.log("input.props: ", input.props);
    expect(input.props.style.paddingTop).toEqual(input.props.style.paddingBottom);
 });

//  test('Composer with one line - Validate top & bottom padding', () => {
//     const { findByPlaceholderText } = render(<ReportActionCompose />);
//     const input = findByPlaceholderText('Write something...');
//     fireEvent.changeText(input, "This is one line");
//     console.log("input.props.style.paddingTop: ", input.props.style.paddingTop);
//     console.log("input.props.style.paddingBottom: ", input.props.style.paddingBottom);
//     expect(input.props.style.paddingTop).toEqual(input.props.style.paddingBottom);
//  });

2. I run the test using the following command:

yarn test --findRelatedTests tests/unit/ComposerPaddingTest.js "Composer padding test"

3. However, I run into the below error:

I qualified the ReportActionCompose class with an export.

Any inputs please?

[Julesssss]

Oh sorry @liyamahendra, I just meant some test steps for QA to follow.

[Julesssss]

Something like this:

---

I think you can just add this to the description:

Test single line padding

• Login in iOS app
• Go to any chat
• Type message in composer and notice the send icon and text written is aligned more to bottom rather than centre vertically
• Text should be proper aligned to vertically centre

Test multi line padding

• Login in iOS app
• Go to any chat
• Type multi-line message in composer and notice the send icon and text written is aligned more to bottom rather than centre vertically
• Text should be proper aligned to vertically centre

[liyamahendra]

@Julesssss haha! My bad I assumed you suggested to write the actual tests.

I just updated the description. Please let me know if there is anything else. Thanks!

[Santhosh-Sellavel]

@liyamahendra Update the tests section too.

[Santhosh-Sellavel]

Also, can you pull the latest changes from the mainbranch thanks!

[liyamahendra]

@Santhosh-Sellavel done!

[Santhosh-Sellavel]

@liyamahendra

PR is failing for me on Android

cc: @Julesssss

[Santhosh-Sellavel]

PR is failing for android

[liyamahendra]

@Santhosh-Sellavel I was able to reproduce the issue you shared on Android. I debugged and solved it. Also tested on all platforms.

iOS

Android

mWeb

Web

Desktop

PS: Please ignore the red border in the screenshot - it was for proper testing of the alignment of the actual text input.

[Santhosh-Sellavel]

Let's test this for a bit more time before we move forward, thanks!

cc: @liyamahendra @Julesssss

[Julesssss]

Web is looking good to me now

[Santhosh-Sellavel]

@liyamahendra
Can you merge with the latest main branch, thanks!

[liyamahendra]

@Santhosh-Sellavel pulled the latest from main!

[Santhosh-Sellavel]

@liyamahendra
Can you update the recordings in the PR description, please?

[Santhosh-Sellavel]

Bug: Compose bottom border is broken in android while typing multiple lines

Screen.Recording.2022-09-04.at.12.20.55.AM.mov

@liyamahendra

[liyamahendra]

@Santhosh-Sellavel that's a very good observation! You've got an eye for quality! :)

Let me look into this please.

[liyamahendra]

@Santhosh-Sellavel The issue doesn't seem to occur on mWeb and iOS. Though I was able to reproduce the issue you shared, I wasn't able to figure out the cause of the issue.

@Santhosh-Sellavel @Julesssss do you've insights into what might be the cause?

[Santhosh-Sellavel]

I think the inner textview may be expanding and rendered over the bottom, try setting a different background color for the textview and investigate it!

@liyamahendra

[liyamahendra]

@Santhosh-Sellavel since the styles & code was looking alright, I tested the app on an actual device, as opposed to emulator and the issue was not reproduced. Looks like its an issue with the rendering on an emulator.

Sharing the screencast from my device.

device.mp4

Please do try on Android and comment if its still an issue.

[Santhosh-Sellavel]

@liyamahendra But I tested on my personal android device only first, then only I took screen recording from simulator for easy convenience.

I'll share you the recording shortly or later today, thanks!

[liyamahendra]

@Santhosh-Sellavel my bad - I was on the wrong branch. Recording is not required please. I do see the issue on the device as well.

[liyamahendra]

@Santhosh-Sellavel I resolved the issue. Please verify when you can.

[Santhosh-Sellavel]

@liyamahendra Can you update the recordings in the PR description, please?

Bump

[Santhosh-Sellavel]

Again adding paddingVertical with alignSelf: 'center'?

Are we sure it won't break anything on other platforms?

[Santhosh-Sellavel]

I think we could move code to StyleUtils and define platform-specific styles.

cc: @Julesssss What do you think?

[liyamahendra]

Are we sure it won't break anything on other platforms?

Yes, I did test out on other platforms!

[Julesssss]

I think we could move code to StyleUtils and define platform-specific styles.

Depends on whether the styling works for all platforms or not... Would prefer to have a single consistent style, if possible

[liyamahendra]

@liyamahendra Can you update the recordings in the PR description, please?

Sorry, what recording are you referring to please? @Santhosh-Sellavel

[Santhosh-Sellavel]

@liyamahendra Can you update the recordings in the PR description, please?

Sorry, what recording are you referring to please? @Santhosh-Sellavel

Screen recording for each platform, performing the steps in QA!

[trjExpensify]

@Santhosh-Sellavel - I think the screenshots in the OP have been updated 6 days ago. @liyamahendra can you confirm? Also, this PR now has conflicts that need to be resolved.

We're so close, let's get this over the line! ❤️

[liyamahendra]

@trjExpensify yes, I did update the description with screencast for all platforms. cc: @Santhosh-Sellavel

[Santhosh-Sellavel]

Please resolve the conflicts

[Santhosh-Sellavel]

@liyamahendra This off holds now, can you merge it with the latest main? So we can get this moving forward, thanks!

[liyamahendra]

@Santhosh-Sellavel I don't have permissions to merge to main. It also says that the PR needs to be approved before merging can happen.

[Julesssss]

Hi @liyamahendra,

@Santhosh-Sellavel means that you should merge the latest changes from main into your branch. This ensures there are no conflicts or new issues that occur since your last commits.

[trjExpensify]

@Santhosh-Sellavel, @liyamahendra has merged main. Have you re-reviewed?

[shawnborton]

Based on the screenshots, iOS still appears to be pushed slightly downward.

[Santhosh-Sellavel]

Why this?

[liyamahendra]

Apart from flex4, I had to add more styling and so styles.inputComposerPadding

[Santhosh-Sellavel]

I'm asking about why defaultValue prop was added here?

[Santhosh-Sellavel]

Based on the screenshots, iOS still appears to be pushed slightly downward.

Agree that's the case on iOS

One Line

Two Lines

Three Lines

[Santhosh-Sellavel]

Android

Weird, not sure why two lines have so much much space

One line

Two Lines

Three Lines

[Santhosh-Sellavel]

Also, one more breakage text should not be centered here right @shawnborton?

[trjExpensify]

Closing this PR as it's no longer a viable direction for the linked issue.