Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Press highlight on a field in expense details overlaps with error message weirdly #34944

Closed
1 of 6 tasks
kavimuru opened this issue Jan 23, 2024 · 12 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kavimuru
Copy link

kavimuru commented Jan 23, 2024

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


Version Number: 1.4.29-0
Reproducible in staging?: y
Reproducible in production?: y
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: @luacmartins
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1705808584924989

Action Performed:

Precondition : Have some violation rules for the expenses in the OD

  1. Create an expense report
  2. Open the receipt to view the details
  3. enter the required fields values (in this case category and tag fields)
  4. Long press the category/tag field to highlight and observe

Expected Result:

There should be no overlapping.

Actual Result:

overlaps the error message weirdly

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b61739c3e4f7859e
  • Upwork Job ID: 1749662510960947200
  • Last Price Increase: 2024-01-23
@kavimuru kavimuru added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot changed the title Press highlight on a field in expense details overlaps with error message weirdly [$500] Press highlight on a field in expense details overlaps with error message weirdly Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01b61739c3e4f7859e

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jan 23, 2024

Proposal

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

Press highlight on a field in expense details overlaps with error message weirdly

What is the root cause of that problem?

In the ViolationMessages component we have -8px of margin top.

<View style={[styles.mtn2, isLast ? styles.mb2 : styles.mb1]}>

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

We should remove margin top.

Result

Result with no negative margin:
Screenshot 2024-01-23 at 11 40 08 AM

Result with children prop and updating some stylings:
Screenshot 2024-01-23 at 11 42 58 AM

What alternative solutions did you explore? (Optional)

  1. I think the best we can do here is to accept children prop inside MenuItem and use ViolationMessages component as a children.
  2. Or we should use error prop in MenuItemWithTopDescription to show the error. We have already used error prop, we can use errorText to show the ViolationMessages.

@jeremy-croff
Copy link
Contributor

Proposal

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

Highlighted area doesn't include the full row when there's an error message

What is the root cause of that problem?

The highlight style doesn't grow into full row when there's an error message.

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

We should update the styling to flex the highlighted area based on the content, inccluding the error message

What alternative solutions did you explore? (Optional)

@Krishna2323
Copy link
Contributor

Proposal Updated

Included more alternative options.

@Krishna2323
Copy link
Contributor

Proposal Updated

Included results screenshots & permalink.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 23, 2024

Proposal

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

overlaps the error message weirdly

What is the root cause of that problem?

We have a negative margin here so the expense details overlaps the error violation message

<View style={[styles.mtn2, isLast ? styles.mb2 : styles.mb1]}>

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

The reason for the negative margin is to decrease the space between the error and the menu description. But I think we can decrease the padding vertical of the menu description by adding wrapperStyle prop into MenuItemWithTopDescription if we can use violation transaction

Ref: #32594 (comment)

<View style={[styles.mtn2, isLast ? styles.mb2 : styles.mb1]}>

What alternative solutions did you explore? (Optional)

We can combine the violation message with line break and pass this into MenuItemWithTopDescription or allow pass array error prop into MenuItemWithTopDescription

@dragnoir
Copy link
Contributor

dragnoir commented Jan 23, 2024

Proposal

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

Error message out of hover frame

What is the root cause of that problem?

Inside MoneyRequestView we are displaying the error message via ViolationMessages but the only hovered component is MenuItemWithTopDescription

{shouldShowCategory && (
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.category') || lodashGet(transaction, 'pendingAction')}>
<MenuItemWithTopDescription
description={translate('common.category')}
title={transactionCategory}
interactive={canEdit}
shouldShowRightIcon={canEdit}
titleStyle={styles.flex1}
onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.CATEGORY))}
brickRoadIndicator={hasViolations('category') ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
/>
{canUseViolations && <ViolationMessages violations={getViolationsForField('category')} />}
</OfflineWithFeedback>
)}

So when the hover/press effect change the color of the background, the zone where the message is displayed will not be effected.

And because ViolationMessages has a negative margin that bring it more to the top, it seem that it's part of the hover zone, but it's not, it's another DIV where no hover effect applied to it.

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

The MenuItemWithTopDescription support displaying error via already existant props errorText.

{!!errorText && (
<FormHelpMessage
isError
shouldShowRedDotIndicator={false}
message={errorText}
style={styles.menuItemError}
/>
)}

/** Error to display below the title */
error?: string;
/** Error to display at the bottom of the component */
errorText?: string;

We should pass the violation message throw it and remove ViolationMessages component.

<MenuItemWithTopDescription
  description={translate('common.category')}
  // ...
  // we can use error or errorText
+ errorText="Error!" // will put violation message here mapped
/>
- {canUseViolations && <ViolationMessages  violations={getViolationsForField('category')}  />}

Picking the righ props and code perfection can be discussed throw PR.

POC:
image

What alternative solutions did you explore?

Edit MenuItem to that props error or errorText support non only string but an array too.

@luacmartins luacmartins self-assigned this Jan 23, 2024
@luacmartins
Copy link
Contributor

Seems like a dupe of #33527. @cead22 can you confirm if we're already fixing this?

@aimane-chnaif
Copy link
Contributor

It's being fixed in #34303

@cead22
Copy link
Contributor

cead22 commented Jan 23, 2024

Yeah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

10 participants