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

[Awaiting C+ offer acceptance] [$500] [MEDIUM] Distance: Improve the delay in the map image showing within the distance request preview on creation #28965

Closed
6 tasks done
trjExpensify opened this issue Oct 5, 2023 · 112 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Distance Wave5-free-submitters External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2

Comments

@trjExpensify
Copy link
Contributor

trjExpensify commented Oct 5, 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. Global create > request money > distance
  2. Enter a start/finish address > next > request
  3. Observe the empty state receipt preview
  4. Wait 4-5s
  5. Observe the receipt preview update to the map thumbnail

Expected Result:

Minimal delay if any in the UI to display the map route. @neil-marcellini had a suggestion to take a snapshot of the route image, or render the route map until the response comes back.

Actual Result:

The response takes a little while and so it's super noticeable that we're waiting on the API request to return the map image. As a result we have this less than premium experience whereby:

  1. You see a thumbnail of the map pre creation on the confirmation screen
  2. Click request and it turns into a green empty state preview for 4-5s
  3. Then it updates and the map route is shown again

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 / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: v1.3.78-1
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
Notes/Photos/Videos: Any additional supporting documentation

mWhZCXcjT7.mp4

Expensify/Expensify Issue URL:
Issue reported by: @trjExpensify
Slack conversation:

View all open jobs on GitHub

CC: @JmillsExpensify

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01fab7e23c87b42e3c
  • Upwork Job ID: 1710051983554887680
  • Last Price Increase: 2024-01-08
  • Automatic offers:
    • aimane-chnaif | Reviewer | 28097325
    • shubham1206agra | Contributor | 28097326
Issue OwnerCurrent Issue Owner: @trjExpensify
@trjExpensify trjExpensify added External Added to denote the issue can be worked on by a contributor Daily KSv2 NewFeature Something to build that is a new item. Bug Something is broken. Auto assigns a BugZero manager. labels Oct 5, 2023
@trjExpensify trjExpensify self-assigned this Oct 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

Current assignee @trjExpensify is eligible for the NewFeature assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Improve the distance request preview on creation [$500] Improve the distance request preview on creation Oct 5, 2023
@melvin-bot melvin-bot bot added the Weekly KSv2 label Oct 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

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

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Oct 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 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

@melvin-bot melvin-bot bot added Help Wanted Apply this label when an issue is open to proposals by contributors and removed Daily KSv2 labels Oct 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Oct 5, 2023
@AmjedNazzal
Copy link
Contributor

I think maybe we should wait for this ongoing refractoring before we make changes to distance.

@shubham1206agra
Copy link
Contributor

shubham1206agra commented Oct 6, 2023

Proposal

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

Improve the distance request preview on the creation.

What is the root cause of that problem?

The map route is missing cause we are waiting for the backend to give the image, which takes 4-5 seconds.

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

Use the component of the map in the MoneyRequestPreview and MoneyRequestView, which are already present and make the map non-interactive by using props.

And remove the optimistic receipt image from CreateDistanceRequest so that we can condition on the hasReceipt property in both MoneyRequestPreview and MoneyRequestView, and we render maps only when no receipt image is present.

What alternative solutions did you explore? (Optional)

Make a snapshot of the map from Mapbox and send the temp image in an optimistic report while executing CreateDistanceRequest so that we can see the map (which is a little different) so that we can static map image till the API returns a response.

Result

Screen.Recording.2023-10-06.at.4.10.16.PM.mov

@shubham1206agra
Copy link
Contributor

@aimane-chnaif If you want a test branch to test the feature, please write it here.
I will do that.
I have chosen not to write too many details just an overview of the idea, cause there are many changes to write and will make the proposal cluttered. I have instead put the video as proof
cc @trjExpensify

@trjExpensify
Copy link
Contributor Author

I think maybe we should wait for this ongoing #26538 (comment) before we make changes to distance.

Oh hm, I don't quite see the correlation myself on this issue, but @tgolen is a smarter man than I, so I'll tag him here for vis!

@tgolen
Copy link
Contributor

tgolen commented Oct 6, 2023

This is only tangentially related to my refactoring (I'm basically rewriting the entire creation flow while trying to keep all the existing behavior). So, it doesn't need to be held on my work, but will create some cleanup for one of us depending on the order the code is merged.

I think it's OK to move forward with exploring solutions, but it would really be nice to halt the actual development so that it builds off of my refactored code instead of the currently existing code.

I have a few thoughts about this issue in general:

  • There are two things that we are waiting for with the MapBox API on the server: first, an image of the route. Second, the distance between the waypoints so that the amount can be calculated. I don't think we can do anything about the second one. We will always need to wait. That makes this issue more about the first part, the map image. If that's the case, then @trjExpensify can you please update the issue title and description to be more clear on what this issue is focusing on?

  • As for doing an optimistic rendering of the map, I think it would be difficult to get an image preview of the map only from the frontend (correct me if I'm wrong... maybe the frontend MapBox API has an easy way of doing that). However, what would be very easy is to embed the actual map, using the optimistic waypoints, into the receipt preview. We would just disable all interactions with the map, and to the user, it should look almost exactly like the eventual receipt image that is returned from the server. The only case where this wouldn't work is in the offline flow, but I think that's OK and expected since the map also wouldn't have rendered in the creation steps.

@shubham1206agra
Copy link
Contributor

@tgolen Can you check my proposal video, and tell me whether this will work or not? #28965 (comment)
I have done this by taking an image snapshot of the map.

@tgolen
Copy link
Contributor

tgolen commented Oct 6, 2023

It functionally looks like what we are expecting, yes. How are you taking an image snapshot of the map?

@shubham1206agra
Copy link
Contributor

For native, there is an inbuilt function and for the web, some manipulation of DOM elements give this (mainly canvas)

@trjExpensify trjExpensify changed the title [$500] Improve the distance request preview on creation [$500] Improve the delay in the map image showing within the distance request preview on creation Oct 6, 2023
@trjExpensify
Copy link
Contributor Author

I think it's OK to move forward with exploring solutions, but it would really be nice to halt the actual development so that it builds off of my refactored code instead of the currently existing code.

Ah okay cool, noted. Once we're aligned on a solution I'll pop a hold in the title and link to the issue/PR. 👍

There are two things that we are waiting for with the MapBox API on the server: first, an image of the route. Second, the distance between the waypoints so that the amount can be calculated. I don't think we can do anything about the second one. We will always need to wait. That makes this issue more about the first part, the map image. If that's the case, then @trjExpensify can you please update the issue title and description to be more clear on what this issue is focusing on?

Yep, agreed. This issue is about the map image. Re: the amount on the preview component, that seems pretty good as it is now. It's the map image showing on confirmation > disappearing > reappearing which is the jarring thing. I've updated, but feel free to suggest further edits if needs be!

The only case where this wouldn't work is in the offline flow, but I think that's OK and expected since the map also wouldn't have rendered in the creation steps.

I agree this okay, for the same reason you mentioned. 👍

@neil-marcellini
Copy link
Contributor

  • As for doing an optimistic rendering of the map, I think it would be difficult to get an image preview of the map only from the frontend (correct me if I'm wrong... maybe the frontend MapBox API has an easy way of doing that). However, what would be very easy is to embed the actual map, using the optimistic waypoints, into the receipt preview. We would just disable all interactions with the map, and to the user, it should look almost exactly like the eventual receipt image that is returned from the server. The only case where this wouldn't work is in the offline flow, but I think that's OK and expected since the map also wouldn't have rendered in the creation steps.

Although doing a snapshot is possible, I agree that rending the map component with interactions disabled is probably best, especially because we already have code to do that. If we're offline we should render the pending map view instead.

@shubham1206agra
Copy link
Contributor

@neil-marcellini I think we should do snapshot as making map component in preview and view may make code a little complex to maintain. If we have showed the map on success request, then the logical way is to create map in optimistic also. But in this case, snapshot is more correct way to go.

@melvin-bot melvin-bot bot added the Overdue label Oct 9, 2023
@trjExpensify trjExpensify changed the title [$500] [MEDIUM] Distance: Improve the delay in the map image showing within the distance request preview on creation [Awaiting Payment 27th April] [$500] [MEDIUM] Distance: Improve the delay in the map image showing within the distance request preview on creation Apr 27, 2024
@trjExpensify trjExpensify added Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels Apr 27, 2024
@shubham1206agra
Copy link
Contributor

@trjExpensify Not a regression but a follow-up feature request which is low on the priority scale right now.

@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 30, 2024

@trjExpensify, @grgia, @shubham1206agra, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@trjExpensify
Copy link
Contributor Author

trjExpensify commented May 1, 2024

@trjExpensify Not a regression but a follow-up feature request which is low on the priority scale right now.

Okay, payment summary as follows:

Offers sent!

@melvin-bot melvin-bot bot removed the Overdue label May 1, 2024
@shubham1206agra
Copy link
Contributor

@trjExpensify Accepted

@trjExpensify
Copy link
Contributor Author

@shubham1206agra paid!

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Daily KSv2 labels May 3, 2024
Copy link

melvin-bot bot commented May 7, 2024

@trjExpensify, @grgia, @shubham1206agra, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@trjExpensify
Copy link
Contributor Author

@aimane-chnaif can you accept the offer, so we can pay this one out?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels May 7, 2024
Copy link

melvin-bot bot commented May 10, 2024

@trjExpensify, @grgia, @shubham1206agra, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@trjExpensify
Copy link
Contributor Author

Aimane is out of office, dropping to weekly

@melvin-bot melvin-bot bot removed the Overdue label May 10, 2024
@trjExpensify trjExpensify added Weekly KSv2 Overdue and removed Daily KSv2 labels May 10, 2024
@melvin-bot melvin-bot bot removed the Overdue label May 10, 2024
@trjExpensify trjExpensify changed the title [Awaiting Payment 27th April] [$500] [MEDIUM] Distance: Improve the delay in the map image showing within the distance request preview on creation [Awaiting C+ offer acceptance] [$500] [MEDIUM] Distance: Improve the delay in the map image showing within the distance request preview on creation May 10, 2024
@aimane-chnaif
Copy link
Contributor

Apologies for the delay. I already have offer accepted in the past.

@trjExpensify
Copy link
Contributor Author

Paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Distance Wave5-free-submitters External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2
Projects
No open projects
Development

No branches or pull requests

9 participants