[HOLD for payment 2022-05-20]iOS - Chat - The maximum height of compose box is 5 instead of 6 lines.

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue was found when executing #7891

Action Performed:

1. Open any chat
2. Type a message with 20-30 lines
3. Verify that the maximum height of compose box

Expected Result:

The maximum height of compose box does not exceed 6 lines.

Actual Result:

The maximum height of compose box is 5 lines.

Workaround:

Visual

Platform:

Where is this issue occurring?

• iOS

Version Number: 1.1.41-1
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/156388405-4c55d506-50f9-4e2c-a9c7-00f43bc8f2e7.mp4

Expensify/Expensify Issue URL:
Issue reported by: Applause

Job Post: https://www.upwork.com/jobs/~013a3b361194e64884

View all open jobs on GitHub

[MelvinBot]

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

[MelvinBot]

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

[sobitneupane]

Proposal:
Add different composer height for android and ios.

App/src/CONST.js

Line 377 in ea691f0

COMPOSER_MAX_HEIGHT: 116,

    COMPOSER_MAX_HEIGHT_ANDROID: 116,
    COMPOSER_MAX_HEIGHT_IOS: 125,

App/src/components/TextInputFocusable/index.android.js

Lines 66 to 74 in ea691f0

	<RNTextInput
	placeholderTextColor={themeColors.placeholderText}
	ref={el => this.textInput = el}
	maxHeight={CONST.COMPOSER_MAX_HEIGHT}
	rejectResponderTermination={false}
	editable={!this.props.isDisabled}
	/* eslint-disable-next-line react/jsx-props-no-spreading */
	{...this.props}
	/>

            <RNTextInput
                placeholderTextColor={themeColors.placeholderText}
                ref={el => this.textInput = el}
-               maxHeight={CONST.COMPOSER_MAX_HEIGHT}
+               maxHeight={CONST.COMPOSER_MAX_HEIGHT_ANDROID}
                rejectResponderTermination={false}
                editable={!this.props.isDisabled}
                /* eslint-disable-next-line react/jsx-props-no-spreading */
                {...this.props}
            />

App/src/components/TextInputFocusable/index.ios.js

Lines 78 to 86 in ea691f0

	<RNTextInput
	placeholderTextColor={themeColors.placeholderText}
	ref={el => this.textInput = el}
	maxHeight={CONST.COMPOSER_MAX_HEIGHT}
	rejectResponderTermination={false}
	editable={!this.props.isDisabled}
	/* eslint-disable-next-line react/jsx-props-no-spreading */
	{...propsToPass}
	/>

            <RNTextInput
                placeholderTextColor={themeColors.placeholderText}
                ref={el => this.textInput = el}
-               maxHeight={CONST.COMPOSER_MAX_HEIGHT}
+               maxHeight={CONST.COMPOSER_MAX_HEIGHT_IOS}
                rejectResponderTermination={false}
                editable={!this.props.isDisabled}
                /* eslint-disable-next-line react/jsx-props-no-spreading */
                {...propsToPass}
            />

[MelvinBot]

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

[MelvinBot]

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

[parasharrajat]

I don't understand the proposal @sobitneupane. What does changing height have to do anything with it? AFAIK, react-native usages DPI.
So if 116px should be equal to 116px phone iPhone even though the PixelRatio is different. Another point is if we use 18px line height of the font on both Android and iOS, then they should consume the same height?

[mdneyazahmad]

Proposal

We need to calculate correct value of maxHeight

App/src/CONST.js

Line 377 in 0e3bbe7

COMPOSER_MAX_HEIGHT: 116,

-   COMPOSER_MAX_HEIGHT: 116,
+   COMPOSER_MAX_HEIGHT: 130, // lineHeight*numberOfLines + marginVertical (20*6 + (5 + 5))

Tested on ios and android, works well.

[mdneyazahmad]

The issue was also there in android

[sobitneupane]

The issue was also there in android

Issue doesn't exist in android in my end.

[mdneyazahmad]

@sobitneupane

Record_2022-03-04-18-01-08.mp4

[TwizzyIndy]

Proposal

1. We should declare maxLines and style in PropTypes. Just like the Desktop view did.

App/src/components/TextInputFocusable/index.ios.js

Lines 15 to 17 in 0e3bbe7

	const propTypes = {
	/** If the input should clear, it actually gets intercepted instead of .clear() */
	shouldClear: PropTypes.bool,

const propTypes = {
    /** Maximum number of lines in the text input */
    maxLines: PropTypes.number,

    /** General styles to apply to the text input */
    // eslint-disable-next-line react/forbid-prop-types
    style: PropTypes.any,

2. We should also have them in defaultProps
   

   App/src/components/TextInputFocusable/index.ios.js

   Lines 40 to 42 in 0e3bbe7

   	const defaultProps = {
   	shouldClear: false,
   	onClear: () => {},

const defaultProps = {
    maxLines: -1,
    style: null,

3. We shouldn't use CONST.COMPOSER_MAX_HEIGHT directly in rendering. So we should put it in state

class TextInputFocusable extends React.Component {
    constructor(props) {
        super(props);

        this.state = {
            textInputMaxHeight: CONST.COMPOSER_MAX_HEIGHT,
        };
    }

4. make a new constant maxHeight which is assigned from CONST.COMPOSER_MAX_HEIGHT. The input text line height should have to be dynamic, so declare propStyle variable using StyleSheet.flatten method which would parse this.props.style in CSS mapping.

    render() {
        // Selection Property not worked in IOS properly, So removed from props.
        const propsToPass = _.omit(this.props, 'selection');
        const maxHeight = CONST.COMPOSER_MAX_HEIGHT;
        const propStyles = StyleSheet.flatten(this.props.style);
        return (
            <RNTextInput

5. maxHeight should get updates from this.state.textInputMaxHeight. We would have to add onContentSizeChange event listener for Text input content size changes. We will get contentSize.height as actual text input content height. Then calculate the lines as Math.round(height / propStyles.lineHeight). And make visibleLines which should less than 6. After all, check whether visibleLines is greater than the maxLines or not. And increase the maxHeight by 20.

EDITED: We should use propStyles.lineHeight instead of 20.

        return (
            <RNTextInput
                placeholderTextColor={themeColors.placeholderText}
                ref={el => this.textInput = el}
                maxHeight={this.state.textInputMaxHeight}
                rejectResponderTermination={false}
                editable={!this.props.isDisabled}
                onContentSizeChange={(event) => {

                    const height = event.nativeEvent.contentSize.height;
                    const lines = Math.round(height / propStyles.lineHeight);
                    const visibleLines = lines < this.props.maxLines ? lines : this.props.maxLines;

                    if (visibleLines >= this.props.maxLines) {
                        this.setState({textInputMaxHeight: maxHeight + propStyles.lineHeight});
                    }
                }}

[mdneyazahmad]

Adding to my initial proposal #7980 (comment) we can alternatively pass the required props and calculate the maxHeight

        const maxLines = this.props.maxLines;
        const { lineHeight, marginVertical } = StyleSheet.flatten(propsToPass.style);
        const maxHeight = lineHeight*maxLines + 2*marginVertical;

[parasharrajat]

Ok, we can calculate the maxHeight of the component based on max lines. I like @mdneyazahmad 's proposals.

But this const is also used in SwipeableView so we need a way to share the value across both components.

cc: @pecanoro
🎀 👀 🎀 C+ reviewed

[pecanoro]

Cool! @mdneyazahmad I assigned you the issue, don't forget to apply for the job in Upwork

[MelvinBot]

📣 @mdneyazahmad You have been assigned to this job by @pecanoro!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[kevinksullivan]

+1 @mdneyazahmad please apply for the job in upwork and I'll hire you there as well. Thanks!

https://www.upwork.com/jobs/~013a3b361194e64884

[parasharrajat]

For now, I would suggest keeping the same behavior as it is working now. We don't have to improve it.

[mdneyazahmad]

The number of lines is not 6 on native device. This will fix it #7980 (comment)

[mdneyazahmad]

@parasharrajat ⬆️

[parasharrajat]

Sorry, I have already given my suggestion. I don't have anything else to ask or suggest. If you are still facing issues, I suggest you ask on public channel.

[mdneyazahmad]

Thank you then should we close this

[parasharrajat]

cc: @pecanoro ⬆️

[pecanoro]

I am a bit confused, is there a reason why we shouldn't do this? I know we should try to make it dynamic, but it seems it's a fixed value already, right?

[parasharrajat]

Discussion is happening around applying the dynamic solution. #7980 (comment) was suggested to be added. My suggestion is to apply this dynamically.

Based on #7980 (comment) , this prop will have two values on the web.

I don't see how replacing the const will use a dynamic value of maxLines prop. @mdneyazahmad is trying to get a solution to do that.

There are two places where this CONST is used. #7980 (comment).

@mdneyazahmad is already hired for the issue and we are awaiting a PR.

[mdneyazahmad]

@parasharrajat This #7980 (comment) is not used in ios and android. That is only for web and desktop.

[parasharrajat]

Sorry, I didn't understand. What is trying to say by referring to this statement? You have pointed that out a few times.

[mdneyazahmad]

Composer/index.js uses this maxLines prop and it depends on device width.

App/src/components/Composer/index.js

Lines 190 to 195 in 9a8291b

	getNumberOfLines(lineHeight, paddingTopAndBottom, scrollHeight) {
	const maxLines = this.props.maxLines;
	let newNumberOfLines = Math.ceil((scrollHeight - paddingTopAndBottom) / lineHeight);
	newNumberOfLines = maxLines <= 0 ? newNumberOfLines : Math.min(newNumberOfLines, maxLines);
	return newNumberOfLines;
	}

But we are not using this maxLines to calculate the number of lines on ios and android we are directly providing the height value in Composer

App/src/components/Composer/index.ios.js

Line 81 in 9a8291b

maxHeight={CONST.COMPOSER_MAX_HEIGHT}

App/src/components/Composer/index.android.js

Line 69 in 9a8291b

maxHeight={CONST.COMPOSER_MAX_HEIGHT}

[parasharrajat]

Ok, Thanks. But even on index.js file, we are using two values 6 and 16. How will you manage that?

[mdneyazahmad]

It is not an issue there it works as expected.

[mdneyazahmad]

This issue only exists in android and ios.

[parasharrajat]

I still think calculating the values dynamically should be the way as hard-coded values will be more error-prone. But @pecanoro I leave the decision to you. Thanks. This is the @mdneyazahmad's suggested change #7980 (comment)

[pecanoro]

So I would usually agree that a dynamic value would be better, but it is already hardcoded so it's not that we are making it worse. Also, since making it dynamic does not seem that straightforward, let's just fix this issue with the proposed solution and maybe in the future we can refactor the whole thing to make it dynamic.

@mdneyazahmad I will assign the job to you!

[pecanoro]

Oh wait, the issue had gone pretty long, you are assigned already! Go ahead with the PR, just make sure it does not cause any regressions.

[kevinksullivan]

PR being worked on

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[pecanoro]

@kevinksullivan This was merged and deployed to staging more than a week ago

[kevinksullivan]

All set and payments sent. Thanks @parasharrajat and @mdneyazahmad !