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

Dev: Money owes preview background color is not changing on hover. #22582

Closed
1 of 6 tasks
kbecciv opened this issue Jul 10, 2023 · 8 comments
Closed
1 of 6 tasks

Dev: Money owes preview background color is not changing on hover. #22582

kbecciv opened this issue Jul 10, 2023 · 8 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Needs Reproduction Reproducible steps needed

Comments

@kbecciv
Copy link

kbecciv commented Jul 10, 2023

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


Action Performed:

  1. Open App > Navigate to any chat.
  2. Press + from composer > Request money > Add amount > Press Next and Press Request.
  3. We can see the Money Owed Preview message in chat, and now Do hover over to the Owed Preview.

Expected Result:

On Hover over the money owes preview message background color of the preview should be changed like the same happens when we hover over on preview on the money details page.

Actual Result:

There is no hover effect happening when hovering over the money owes preview so that the background color of the preview and hover color overlap.

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: Dev 1.3.38-4

Reproducible in staging?: n/a
Reproducible in production?: n/a
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
Notes/Photos/Videos: Any additional supporting documentation

no-hover-effect.mov

Expensify/Expensify Issue URL:
Issue reported by: @jayeshmangwani
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688983671141219

View all open jobs on GitHub

@kbecciv kbecciv added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Jul 10, 2023
@esh-g
Copy link
Contributor

esh-g commented Jul 10, 2023

Proposal

Please re-state the issue

Money owe preview background color is not changing on hover

What is the root cause of that problem?

There is no hover style to highlight the preview box which is present in split preview.

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

We should just add the hover styles so that the box is highlighted on hover in the following line:

<View style={styles.iouPreviewBox}>

<View style={[styles.iouPreviewBox, props.isHovered ? styles.iouPreviewBoxHover : undefined]}>

Just like we do here:

containerStyles={[styles.cursorPointer, props.isHovered ? styles.iouPreviewBoxHover : undefined, ...props.style]}

What other alternative approaches did you explore?

N/A

@melvin-bot
Copy link

melvin-bot bot commented Jul 10, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 10, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@Nikhil-Vats
Copy link
Contributor

Proposal

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

Request Money ReportPreview's background color does not change on hover.

What is the root cause of that problem?

When we are rendering ReportPreview in ReportActionItem, we pass isHovered prop -

<ReportPreview
iouReportID={ReportActionsUtils.getIOUReportIDFromReportActionPreview(props.action)}
chatReportID={props.report.reportID}
action={props.action}
isHovered={hovered}
contextMenuAnchor={popoverAnchorRef}
checkIfContextMenuActive={toggleContextMenuFromActiveReportAction}
/>

But no such prop even exists in ReportPreview so there is no background color change based on the hover state.

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

There is no use of isHovered prop in ReportPreview so we don't need to pass it, instead we should add a new prop containerStyles in ReportPreview and based on hover state in ReportActionItem, we should add hover background color -

<ReportPreview
    iouReportID={ReportActionsUtils.getIOUReportIDFromReportActionPreview(props.action)}
    chatReportID={props.report.reportID}
    action={props.action}
    containerStyles={[hovered ? styles.iouPreviewBoxHover : undefined]}
    contextMenuAnchor={popoverAnchorRef}
    checkIfContextMenuActive={toggleContextMenuFromActiveReportAction}
/>

Then in ReportPreview, we need to change line 109 to use new containerStyles prop -

<View style={[styles.iouPreviewBox, ...props.containerStyles]}>

P.S. - We can also keep the type of containerStyles prop as an object rather than an array.

This is consistent with what we are doing for IOUPreview. In ReportActionItem, we render MoneyRequestAction and pass it isHovered prop, and in MoneyRequestAction based on isHovered, we pass containerStyles to IOUPreview -

<IOUPreview
iouReportID={props.requestReportID}
chatReportID={props.chatReportID}
isBillSplit={isSplitBillAction}
action={props.action}
contextMenuAnchor={props.contextMenuAnchor}
checkIfContextMenuActive={props.checkIfContextMenuActive}
shouldShowPendingConversionMessage={shouldShowPendingConversionMessage}
onPreviewPressed={onIOUPreviewPressed}
containerStyles={[styles.cursorPointer, props.isHovered ? styles.iouPreviewBoxHover : undefined, ...props.style]}
isHovered={props.isHovered}
/>

We do pass isHovered too in line 152 but that is used in IOUPreview only for MultipleAvatars component styling which is not used in ReportPreview so we don't need isHovered in ReportPreview.

I noticed one more margin issue in `ReportPreview`. See screenshot -

Notice that there is margin before image and before IOUPreview in chat but for ReportPreview (last message) there is no margin and it is right below the user name.
Screenshot 2023-07-10 at 9 03 00 PM

To fix this, we can either also pass styles.mt2 in containerStyles from ReportActionItem or we can directly use styles.mt2 in ReportPreview which are also used for image and IOUPreview.

What alternative solutions did you explore? (Optional)

Alternatively, we can add the isHovered prop in ReportPreview (we are already passing it but it doesn't exist) and then based on its value we can add hover background styles in ReportPreview itself as explained above.

Result -

Screen.Recording.2023-07-10.at.9.06.57.PM.mov

@jayeshmangwani
Copy link
Contributor

jayeshmangwani commented Jul 12, 2023

@kevinksullivan This issue was solved yesterday in this PR; we can close this issue

@jayeshmangwani
Copy link
Contributor

@kevinksullivan Can you please confirm if I am eligible for reporting bonus here, the issue was reported before the PR created

@kevinksullivan
Copy link
Contributor

If the issue was solved elsewhere then we don't pay reporting bonus

@jayeshmangwani
Copy link
Contributor

Yes, I understand that, but I was confirming because this issue was created before this issue which solves that current issue in this PR. Thanks!

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 Needs Reproduction Reproducible steps needed
Projects
None yet
Development

No branches or pull requests

5 participants