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

Web - The Cursor displayed as enabled when hovering over the amount, description, merchant, or date for the paid request money. #26441

Closed
1 of 6 tasks
kbecciv opened this issue Aug 31, 2023 · 19 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering

Comments

@kbecciv
Copy link

kbecciv commented Aug 31, 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 the website
  2. Click the + icon from LHN
  3. Click request money
  4. Enter amount
  5. Select Workspace from the list if there is no workspace create one before requesting a money
  6. Confirm request money
  7. Open the IOU report
  8. Pay with "Pay Elsewhere"
  9. Open edit request
  10. Hover on the amount, description, merchant, and date
  11. Observe the result

Expected Result:

The Cursor should display as disabled when hovering over the amount, description, merchant, or date for the paid request money.

Actual Result:

The Cursor displayed as enabled when hovering over the amount, description, merchant, or date for the paid request money.

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: 1.3.60.1
Reproducible in staging?: y
Reproducible in production?:
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

Screenshare.-.2023-09-01.12_22_45.AM.mp4
Recording.4145.mp4

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

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Aug 31, 2023
@OSBotify
Copy link
Contributor

👋 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.

@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2023

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

@kbecciv kbecciv changed the title Web - IOU report fields are not grey out after Pay with "Pay Elsewhere" Web - The Cursor should display as disabled when hovering over the amount, description, merchant, or date for the paid request money Aug 31, 2023
@kbecciv kbecciv changed the title Web - The Cursor should display as disabled when hovering over the amount, description, merchant, or date for the paid request money Web - The Cursor displayed as enabled when hovering over the amount, description, merchant, or date for the paid request money. Aug 31, 2023
@Krishna2323
Copy link
Contributor

Krishna2323 commented Aug 31, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.
Web - Workspace's badge is not appeared when hovering over it on thread

What is the root cause of that problem?

This is coming from this PR

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

Option 1

We need to add disabled={isSettled && !canEdit} to all MenuItemWithTopDescription element

Option 2

We can only show the disabled cursor without greying the text, like we do with complete tasks.
shouldGreyOutWhenDisabled={false}

Option 3

If we don't want to grey out text and don't want to show the disabled cursor, we can pass focusable={canEdit}, so it doesn't add focus to task details if it has been paid.

Focus

Screen.Recording.2023-09-01.at.3.28.57.AM.mov

@luacmartins
Copy link
Contributor

luacmartins commented Aug 31, 2023

I don't think this is a blocker. There's no functionality impacted here and it's a small cursor UI thing.

@luacmartins luacmartins removed the DeployBlockerCash This issue or pull request should block deployment label Aug 31, 2023
@luacmartins
Copy link
Contributor

IMO the way we have here is better, so I think we should do nothing, but I'll let @shawnborton chime in here.

@luacmartins luacmartins added the Bug Something is broken. Auto assigns a BugZero manager. label Aug 31, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Aug 31, 2023

@luacmartins, we can show the disabled cursor without greying the text. We do same for the completed task and I think we should be consistent with that.

Screen.Recording.2023-09-01.at.3.44.49.AM.mov

@luacmartins
Copy link
Contributor

Yea, I know we can. I just dislike the disabled cursor haha but then again, that's up to design

@Krishna2323
Copy link
Contributor

@luacmartins, yeah, that's what I meant, whatever the design is, we should be consistent.

@Krishna2323
Copy link
Contributor

@luacmartins, we will have one more issue if we do not disable it, it will be focusable using tab/f11 key.

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@mallenexpensify mallenexpensify added Daily KSv2 and removed Hourly KSv2 labels Sep 4, 2023
@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2023
@mallenexpensify
Copy link
Contributor

Bumping this to Daily per Carlo's comment

I don't think this is a blocker. There's no functionality impacted here and it's a small cursor UI thing.

Also from Carlos

IMO the way we have here is better, so I think we should do nothing, but I'll let @shawnborton chime in here.

I'm not sure I 100% grasp the bug. I'm assuming it's that the cursor shows like this
image

when it should (maybe) show like this
image

@mallenexpensify mallenexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 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 Sep 5, 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

@mallenexpensify
Copy link
Contributor

@kevinksullivan I'm off this week, can you please keep 👀 on this then I'll snag it back on Monday? Thx

@shawnborton
Copy link
Contributor

I'm a little lost on this one but cc'ing @mountiny here - I think having these rows display as disabled was a mistake, and thus we want to revert them to appear like read-only push-to-page rows (with no caret) and thus we don't need a disabled cursor.

@mountiny
Copy link
Contributor

mountiny commented Sep 6, 2023

Yeah I dont understand this, we did have disabled and that was temporary and its fixed, now it looks normal, there is no hover effect, no change in cursor and no chevron so its clear you cannot clicked. This is intentional and not a bug

Screen.Recording.2023-09-06.at.18.02.14.mov

@mountiny mountiny closed this as completed Sep 6, 2023
@Krishna2323
Copy link
Contributor

Krishna2323 commented Sep 6, 2023

@mountiny, we also need set focusable={canEdit}, otherwise the task view details will be focusable even if they are not editable and we also need to change it on task view.

Should I create a separate issue for this?

Monosnap.screencast.2023-09-06.20-34-32.mp4

@mountiny
Copy link
Contributor

mountiny commented Sep 7, 2023

Should I create a separate issue for this?

@Krishna2323 yeah this issue as far as I can see from the title is not about tasks, 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 Engineering
Projects
None yet
Development

No branches or pull requests

9 participants