[HOLD for payment 2024-05-03] [$250] Room - Description Error Message is overlapped by highlight

[kavimuru]

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

---

Version Number: 1.4.63-0
Reproducible in staging?: y
Reproducible in production?: No, new feature to show the error message
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

1. Access staging.new.expensify.com
2. Sign into a valid account
3. Go to "Start Chat" > Room
4. Input a very long room description like:

# we can close this to focus on more urgent issue this is just a minor viewing discrepancy

~we can close this to focus on more urgent issue this is just a minor viewing discrepancy~

we can close this to focus on more urgent issue this is just a minor viewing discrepancy

> *we can close this to focus on more urgent issue this is just a minor viewing discrepancy*

> *we can close this to focus on more urgent issue this is just a minor viewing discrepancy*"

5. Highlight the workspace name section on Web or Desktop

Expected Result:

User expects the highlight to NOT interfere with the error message

Actual Result:

The highlight is overlapping the error message

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6454141_1713466549486.Room_description_Error_message_is_being_overlapped_by_the_hihglight.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016751dd795b5647cd
• Upwork Job ID: 1782572739401596928
• Last Price Increase: 2024-04-23
• Automatic offers:
• Pujan92 | Reviewer | 0
• bernhardoj | Contributor | 0

[melvin-bot]

Triggered auto assignment to @marcaaron (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[kavimuru]

We think this bug might be related to #vip-vsb

[kavimuru]

@trjExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[thienlnam]

Doesn't seem like this needs to be a blocker since it only happens if you max out the description

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The highlight is overlapping the error message

What is the root cause of that problem?

We're using the same maxHeight for both text input and container. In this case it's 115px

App/src/components/TextInput/BaseTextInput/index.tsx

Line 290 in 68e6962

style={[containerStyles]}

App/src/components/TextInput/BaseTextInput/index.tsx

Line 300 in 68e6962

autoGrowHeight && styles.autoGrowHeightInputContainer(textInputHeight, variables.componentSizeLarge, typeof maxHeight === 'number' ? maxHeight : 0),

When the errorText appear, the height does not increase because maxHeight of container and input are the 115px and there's no space for errorText

What changes do you think we should make in order to solve the problem?

These two value should be different

In

App/src/components/TextInput/BaseTextInput/index.tsx

Line 258 in 68e6962

const maxHeight = StyleSheet.flatten(containerStyles).maxHeight;

we get the maxHeight for text input so it should be decreased by 28px (mt: 8px, height: 20px) (We can consider marginBottom but I think it's not necessary). This value can be adjusted

    const maxHeight = StyleSheet.flatten(containerStyles).maxHeight - 28;

Or we can calculate the height of errorText dynamically

We also need to update containerStyles.maxHeight to keep the current behavior.

What alternative solutions did you explore? (Optional)

NA

Result

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Room description error message is overlapped with the workspace field highlight.

What is the root cause of that problem?

We set the containerStyle of the description field with a maxHeight of 115.

App/src/pages/workspace/WorkspaceNewRoomPage.tsx

Line 296 in bec1c8e

containerStyles={[styles.autoGrowHeightMultilineInput]}

The containerStyle is applied to this View which contains the text input and the error message

App/src/components/TextInput/BaseTextInput/index.tsx

Lines 287 to 290 in bec1c8e

	return (
	<>
	<View
	style={[containerStyles]}

<View>
    <Input>
    <ErrorMessage>
</View>

The maxHeight from containerStyle is also being used to set the max height for the input height itself. So, when the input reaches its max height and the error shows, the error message will overlap with the element below it.

What changes do you think we should make in order to solve the problem?

I think we shouldn't pass the max height of the auto grow height to the containerStyle as they are being used differently. So, I propose to have a new props called maxAutoGrowHeight and we start passing the max height to the new props.

maxAutoGrowHeight={115}

Then, we can replace all usages of maxHeight with maxAutoGrowHeight in BaseTextInput component.

[melvin-bot]

@trjExpensify, @marcaaron Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016751dd795b5647cd

[melvin-bot]

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

[trjExpensify]

CC: @jasperhuangg for vis, looks like your PR was being previewed in the OP on this.

[Pujan92]

Thanks for the proposals.

@Krishna2323 and @tienifr proposed adjusting margin-bottom/max-height by considering an error message to be of a single line. But the error message can be longer so their proposals aren't solving the issue at the root level. Though @tienifr mentioned calculating error text height dynamically, I don't think it is the best approach here.

According to me, we should not restrict the container height as it is also responsible for rendering the error message which is correctly pointed out in @bernhardoj's proposal. We can move here with @bernhardoj's proposal where they suggested passing a new prop maxAutoGrowHeight which will be responsible for controlling height excluding the container View.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @marcaaron is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @Pujan92 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[bernhardoj]

PR is ready

cc: @Pujan92

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.66-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix new room description field error message is overlapped by other fields highlight #40851

If no regressions arise, payment will be issued on 2024-05-03. 🎊

For reference, here are some details about the assignees on this issue:

• @Pujan92 requires payment automatic offer (Reviewer)
• @bernhardoj requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@Pujan92] The PR that introduced the bug has been identified. Link to the PR:
• [@Pujan92] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@Pujan92] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@Pujan92] Determine if we should create a regression test for this bug.
• [@Pujan92] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Pujan92]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@Pujan92] The PR that introduced the bug has been identified. Link to the PR: feat(expensify-card): add get physical card button and routes #28453
• [@Pujan92] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/28453/files#r1590294940
• [@Pujan92] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: NA
• [@Pujan92] Determine if we should create a regression test for this bug. Yes
• [@Pujan92] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Steps

1. Open a new room page
2. Enter a long description until it exceeds the max characters limitation
3. Verify the error shown doesn't overlap with the other bottom content

[trjExpensify]

It's a pretty niche bug to get to the max character limit with a description input, so I'm going to forgo the regression test for this.

Payment summary as follows:

• $250 to @bernhardoj for the fix
• $250 to @Pujan92 for the C+ review

Paid, closing.. thanks ya'll!