[$250] Chat - Height of edit field is different for mWeb and native app @thesahindia

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Launch the app
2. Navigate to any chat
3. Send a message
4. Long press on it > Edit
5. Keep pressing enter and check the max height

Expected Result:

The height should be consistent

Actual Result:

The height isn't consistent

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.12.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @thesahindia

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664197609260269

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @strepanier03 (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[Puneet-here]

Proposal

We need to keep the max lines 6 for isSmallScreenWidth and 16 when it's false

https://github.com/Expensify/App/blob/f84651934726d25e720047839c4ed750eea111b6/src/pages/home/report/ReportActionItemMessageEdit.js#L201

maxLines={this.props.isSmallScreenWidth ? 6 : 16}

we can keep add them in CONST.js

I think we should also check if it's a touch screen by canUseTouchScreen() we should also check the same here https://github.com/Expensify/App/blob/f84651934726d25e720047839c4ed750eea111b6/src/pages/home/report/ReportActionCompose.js#L144

[strepanier03]

Triage notes:

This issue is repeatable.

Tested this on my Android using the new.expensify mobile app.

I was able to have a height much greater than 6 or 16, it seemed near infinite and I could keep increasing the height repeatedly.

[strepanier03]

I think even with a proposal, this should go to Eng according ot the linked SO. There's multiple moving parts to our transition to BZ and I think this Triage process will go away in it's current form soon, a DD was just released for reviews regarding it.

Based on this thread in Slack, I think I should still triage as normal for now, with this going to eng who can put the external label on it as needed??

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Current assignee @roryabraham is eligible for the External assigner, not assigning anyone new.

[roryabraham]

@Puneet-here's proposal looks good to me, but I'll let @rushatgabhane buddy-check it. I'm not sure if we need to check canUseTouchScreen or if just screen width is sufficient here.

[rushatgabhane]

@Puneet-here question: In message edit, why is the max line height 6 for native? Shouldn't it be 16 because we're setting it over here?

https://github.com/Expensify/App/blob/f84651934726d25e720047839c4ed750eea111b6/src/pages/home/report/ReportActionItemMessageEdit.js#L201

Also, I'm curious why we should use canUseTouchScreen(), is it because it's mWeb only?

[Puneet-here]

@Puneet-here question: In message edit, why is the max line height 6 for native? Shouldn't it be 16 because we're setting it over here?

It's probably because of the styling

Also, I'm curious why we should use canUseTouchScreen(), is it because it's mWeb only?

I think it's good to use canUseTouchScreen() and isSmallScreenWidth because this way the height of composer won't change for desktop when we decrease the screen size

[rushatgabhane]

It's probably because of the styling

@Puneet-here I think we need to dive deeper then. Like which specific style is changing maxLine behavior for native only?

I think it's good to use canUseTouchScreen() and isSmallScreenWidth because this way the height of composer won't change for desktop when we decrease the screen size

Oh, I get you. Desktop app is rarely resized to small width. And I think it's okay to change the height on desktop when the screen size is reduced to simplify the code a bit.

[sonialiap]

Job is posted to Upwork

[Puneet-here]

@Puneet-here I think we need to dive deeper then. Like which specific style is changing maxLine behavior for native only?

I was a little bit busy, I will check this today.

[Puneet-here]

Apologies, I just realised that I had closed the tab without really pressing the comment button earlier.

So we have platform specific file for composer and because we are passing maxHeight, passing maxLines has no effect on it https://github.com/Expensify/App/blob/83676c1036299ffecbf82f4b829a7f8cbd741cd7/src/components/Composer/index.ios.js#L104 https://github.com/Expensify/App/blob/83676c1036299ffecbf82f4b829a7f8cbd741cd7/src/components/Composer/index.android.js#L99

[rushatgabhane]

You're right! maxLines internally sets the numberOfLines. And numberOfLines is an android only prop (docs).

So for Native, we're using maxHeight.

We need to keep the max lines 6 for isSmallScreenWidth and 16 when it's false

Sounds good.

@roryabraham @Puneet-here's proposal LGTM! :) 🎀 👀 🎀 C+ reviewed

[rushatgabhane]

@roryabraham do you think a cleaner solution would be to implement maxLines for native using maxHeight?

This way, Composer will have a single interface to change it's maximum number of lines.

[rushatgabhane]

bump on https://github.com/Expensify/App/issues/11694#issuecomment-1291451773 @roryabraham

[melvin-bot[bot]]

@sonialiap, @rushatgabhane, @roryabraham Whoops! This issue is 2 days overdue. Let's get this updated quick!

[roryabraham]

@roryabraham do you think a cleaner solution would be to implement maxLines for native using maxHeight?

I think a cleaner solution would probably be to implement the maxLines prop in React Native for iOS and use maxLines consistently across platforms.

[rushatgabhane]

Perfect, thanks @roryabraham

@Puneet-here feel free to submit an update proposal applying this suggestion

[roryabraham]

Overall I feel like:

1. The platform differences of the Composer have diverged more than they should have, such that the differences between the platforms aren't clear
2. There is a lot of custom logic in the Composer component, much of which is probably intentional, but a lot of the context behind that logic is not very transparent. Some automated tests which cover the behaviors that we expect would go a long way into preserving the intention of the component.
3. What is even happening with isComposerFullSize?
   • Why do we have three separate values for isFullComposerAvailable, setIsFullComposerAvailable, and isComposerFullSize?
   • isComposerFullSize is actually persisted in Onyx on a report level: https://github.com/Expensify/App/blob/0e147426759c9cf4878479f3ffeb69e54249aeb6/src/pages/home/ReportScreen.js#L341-L343, is that something we actually want to persist between sessions or are we just using that as a way to manage UI state?
   • Do we actually need a separate value for isFullComposerAvailable? It seems like it's just shadowing isComposerFullSize.
4. It seems like all of this boils down to maxLines, which we could use directly across all platforms if it was implemented in React Native iOS.
5. Why is maxLines smaller for small screens? Is this because we have a software keyboard that takes up screen height? If so, why don't we determine maxLines based on whether the software keyboard is open, rather than based on platform or screen width?

I think the wisest course of action would be:

• To do a Full Throttle (FT) or Slack meeting for the Composer component across platforms, to identify the behaviors we expect.
• (optional, but ideal) Write some automated tests to cover the expected behavior
• Update the Composer component to address this issue and some of the above concerns with as simple and cross-platform an implementation as possible, implementing maxLines in React Native for iOS if needed.

What do people think?

[melvin-bot[bot]]

@sonialiap, @rushatgabhane, @roryabraham Whoops! This issue is 2 days overdue. Let's get this updated quick!

[roryabraham]

Didn't have time to do a deep-dive on this myself yet, but I did take the opportunity to do one pass on cleanup of the Composer component in this PR. It's not much, but it's an easy win and a step in the right direction.

Long-term for this specific bug I think we probably want to implement maxLines on iOS as well as Android in RN.

[sonialiap]

Price doubled 🙌

[roryabraham]

This PR turned out to be not quite ready yet. An upstream fix is needed, but then it will be ready

[tjferriss]

Following instructions for the weekly update chore:

@roryabraham this is one of the oldest issues in the /App repo. To help us clear out the large backlog of bugs, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is
• For any that can't, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #bug-zero

[trjExpensify]

Ah, just commented here in thread. I’m changing the label to Internal and removing Help wanted.

[melvin-bot[bot]]

@sonialiap, @rushatgabhane, @roryabraham Huh... This is 4 days overdue. Who can take care of this?

[roryabraham]

Ah, just commented here in thread. I’m changing the label to Internal and removing Help wanted.

Sorry, I think there was a miscommunication here @trjExpensify. The PR I put up was a step in the right direction, but not a solution to this issue.

Also the upstream fix was merged in our fork, but in the actual react-native-web upstream, the maintainer has stopped accepting pull requests in the VirtualizedList components. I think the plan is to move VirtualizedList into a separate package of React Native. We need to have a conversation about what to do here.