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

[$250] Distance - Distance label is not positioned within the map view #56531

Open
4 of 8 tasks
mitarachim opened this issue Feb 7, 2025 · 38 comments
Open
4 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@mitarachim
Copy link

mitarachim commented Feb 7, 2025

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: 9.0.95-0
Reproducible in staging?: Yes
Reproducible in production?: Unable to check
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: Exp
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Mac 15.0 / Chrome
App Component: Money Requests

Action Performed:

  1. Go to staging.new.expensify.com
  2. Open FAB > Create expense > Distance.
  3. Enter the following waypoints:

Start - 88 Kearny Street
First Stop - Golden Gate Bridge
Second stop - Telegraph Hill

Expected Result:

The distance label will be positioned within the map view.

Actual Result:

The distance label is not positioned within the map view.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6736043_1738920872146.20250207_173156.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021890346455667275066
  • Upwork Job ID: 1890346455667275066
  • Last Price Increase: 2025-02-14
  • Automatic offers:
    • suneox | Contributor | 106124815
Issue OwnerCurrent Issue Owner: @suneox
@mitarachim mitarachim added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 DeployBlockerCash This issue or pull request should block deployment labels Feb 7, 2025
Copy link

melvin-bot bot commented Feb 7, 2025

Triggered auto assignment to @abekkala (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.

Copy link

melvin-bot bot commented Feb 7, 2025

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

Copy link

melvin-bot bot commented Feb 7, 2025

💬 A slack conversation has been started in #expensify-open-source

Copy link
Contributor

github-actions bot commented Feb 7, 2025

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

@mjasikowski
Copy link
Contributor

mjasikowski commented Feb 7, 2025

Not a blocker - minor UI issue. Caused by #55517

@truph01 @shubham1206agra can you look into that please?

@mjasikowski mjasikowski added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Feb 7, 2025
@truph01
Copy link
Contributor

truph01 commented Feb 7, 2025

I’m currently investigating the issue. The problem arises because determining the correct position for the distance label is challenging. I’ll look into improving this.

@truph01
Copy link
Contributor

truph01 commented Feb 10, 2025

This issue is already known, as we mentioned during the implementation of PR #55517. In that PR, we encountered the issue and here’s our perspective on it:

I almost pointed this out, but since the path changes based on stops and zoom level, it seems like something we’ll never be able to get 100% perfect.

@melvin-bot melvin-bot bot added the Overdue label Feb 10, 2025
@mjasikowski
Copy link
Contributor

Thanks @truph01. @Expensify/design is it OK if we consider this a mild UX inconvenience that is very difficult to fix and just learn to live with it?

@melvin-bot melvin-bot bot removed the Overdue label Feb 10, 2025
@dannymcclain
Copy link
Contributor

It would be nice for it to at least show up inside the view... But I'm not sure how strongly I feel. Let's see what @dubielzyk-expensify thinks.

@shawnborton
Copy link
Contributor

Yeah agree, I think ideally we should display it within the overall map view if we can.

@dubielzyk-expensify
Copy link
Contributor

Yeah this feels worth solving. No point having the label if we can't guarantee it showing up

@mjasikowski
Copy link
Contributor

Thanks all! @truph01 try to improve the label positioning as much as you can. Let's aim to display it reliably.

@truph01
Copy link
Contributor

truph01 commented Feb 12, 2025

It would be nice for it to at least show up inside the view

I'm currently looking into the issue, but the visibility of the distance label depends on the zoom level. As shown in the video below, the visibility changes according to the zoom value:

Screen.Recording.2025-02-12.at.17.57.26.mov

Ensuring the distance label stays visible at all zoom levels is difficult. In my opinion, the only solution is to anchor the distance label to a fixed position on the map, such as the bottom left corner. @dannymcclain

@dannymcclain
Copy link
Contributor

I don't think it needs to stay visible at ALL zoom levels—it just needs to be visible on the initial zoom level. So basically, when you first land on the map you should be able to see it in the view, and after that, if you go messing with the map/zoom level, that's on you. cc @Expensify/design for a gut check on that.

@shawnborton
Copy link
Contributor

Yeah, that's my thinking too. Is there a way to just try to center it up within the map view? In the latest example it seems like it's anchored to the middle stop, instead of just trying to be vertically and horizontally centered to the map line/view.

@dubielzyk-expensify
Copy link
Contributor

Agree with both the designers here. I think if you want a good reference, then open Google Maps and look up directions from A -> B and use set it to Driving. They might have more complicated calculations that we might not want or Mapbox might not provide, so like the above said it mostly just needs to look deliberate for the first zoom level

@parasharrajat
Copy link
Member

parasharrajat commented Feb 13, 2025

So we are currently using margin to Offset the label from the path but that does not work well so I removed that in the #56531 (comment) comment. We can use anchorOffset to position the label but that it will further calculations to determine the direction of offset which is very tricky. The path or direction can be of any shape so we can't achieve perfection in terms of offsetting the label but I think we can try to determine the direction and apply some offset.

Image

P = distance Label on Direction path.
C = Center of the Map View.

We move/offset the label in the opposite direction of the path to the center direction. But I can't guarantee whether this will work as it is just a speculation.

Copy link

melvin-bot bot commented Feb 14, 2025

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

@parasharrajat
Copy link
Member

@mjasikowski What do you think about the #56531 (comment)? We have design team approval.

Do you think, I can implement this? #56531 (comment)

@mjasikowski
Copy link
Contributor

mjasikowski commented Feb 14, 2025

@parasharrajat go ahead! I'll mark this issue as external. Please submit a standard proposal for the fix, @suneox will review and I'll assign the issue to you

@melvin-bot melvin-bot bot removed the Overdue label Feb 14, 2025
@mjasikowski mjasikowski added the External Added to denote the issue can be worked on by a contributor label Feb 14, 2025
@melvin-bot melvin-bot bot changed the title Distance - Distance label is not positioned within the map view [$250] Distance - Distance label is not positioned within the map view Feb 14, 2025
Copy link

melvin-bot bot commented Feb 14, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 14, 2025
Copy link

melvin-bot bot commented Feb 14, 2025

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 14, 2025
Copy link

melvin-bot bot commented Feb 14, 2025

📣 @suneox 🎉 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 📖

@parasharrajat
Copy link
Member

parasharrajat commented Feb 14, 2025

Proposal

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

The distance label is going out of view.

What is the root cause of that problem?

We are anchoring the distance label on the mid point of the direction path/cooridates. Due that nature of the direction path, they can be any shape where middest point can be anywhere in the map view. This could lead to distance level going out of the view.

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

To fix this I suggest that we determine a coordinate along the direction path which is closest to the center of map viewport/visible area. This is fast and based on pure Math.

This can be achieved by this.

  1. First calculate the distance of a point from the center point using Haversine formula.
function calculateDistance(lat1: number, lon1: number, lat2: number, lon2: number): number {
    const R = 6371; // Radius of the earth in km
    const dLat = deg2rad(lat2 - lat1);
    const dLon = deg2rad(lon2 - lon1);
    const a = Math.sin(dLat / 2) * Math.sin(dLat / 2) + Math.cos(deg2rad(lat1)) * Math.cos(deg2rad(lat2)) * Math.sin(dLon / 2) * Math.sin(dLon / 2);
    const c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a));
    const d = R * c; // Distance in km
    return d;
}

function deg2rad(deg: number): number {
    return deg * (Math.PI / 180);
}
  1. Then we determine the closest point in the line segments which are created by taking two points from the direction coordinates at a time from the center . This is pure math...Consider that two points make a line and then you draw a streight line from the center to this line. where this line is smallest is the winner.
function closestPointOnSegment(point: LatLng, startPoint: Coordinate, endPoint: Coordinate): LatLng {
    const x0 = point.lng;
    const y0 = point.lat;
    const x1 = startPoint[0];
    const y1 = startPoint[1];
    const x2 = endPoint[0];
    const y2 = endPoint[1];

    const dx = x2 - x1;
    const dy = y2 - y1;

    if (dx === 0 && dy === 0) {
        return {lng: x1, lat: y1};
    }

    const t = ((x0 - x1) * dx + (y0 - y1) * dy) / (dx * dx + dy * dy);

    let closestX;
    let closestY;
    if (t < 0) {
        closestX = x1;
        closestY = y1;
    } else if (t > 1) {
        closestX = x2;
        closestY = y2;
    } else {
        closestX = x1 + t * dx;
        closestY = y1 + t * dy;
    }

    return {lng: closestX, lat: closestY};
}
  1. Now, we need to combine both of above together to determine the closest point among the line segments created by direction coordinates. Takeing two subsequent direction coordinatees at a time and then finding the closet point on that line. Then comparing the distance of this point from all lines and taking the one that has smallest distance.
function findClosestCoordinateOnLine(map: MapRef, lineCoordinates: Coordinate[]): Coordinate | null {
    if (!lineCoordinates || lineCoordinates.length < 2) {
        return null;
    }

    const viewportCenter = getViewportCenter(map);
    if (!viewportCenter) {
        return null;
    }

    let closestPointOnLine: Coordinate | null = null;
    let minDistance = Infinity;

    for (let i = 0; i < lineCoordinates.length - 1; i++) {
        const startPoint = lineCoordinates.at(i);
        const endPoint = lineCoordinates.at(i + 1);

        const closestPoint = closestPointOnSegment(viewportCenter, startPoint, endPoint);

        const distance = calculateDistance(viewportCenter.lat, viewportCenter.lng, closestPoint.lat, closestPoint.lng);

        if (distance < minDistance) {
            minDistance = distance;
            closestPointOnLine = [closestPoint.lng, closestPoint.lat];
        }
    }

    return closestPointOnLine;
}


function getViewportCenter(map: MapRef): LatLng | null {
    const bounds = map.getBounds();
    return bounds?.getCenter();
}
  1. Now, we hook this logic to calculate the distanceSymbolCoorinate whenever waypoints and directionCoordinates change.

const distanceSymbolCoorinate = useMemo(() => {

It could look like this

const distanceSymbolCoorinate = useMemo(() => {
            const length = directionCoordinates?.length;
            // If the array is empty, return undefined
            if (!length) {
                return undefined;
            }

            if (!mapRef) {
                return;
            }
            return findClosestCoordinateOnLine(mapRef, directionCoordinates);
        }, [directionCoordinates, mapRef]);
  1. Further UI cleanup and adjustments if any will be done on PR.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can add unit tests to the above functions.

What alternative solutions did you explore? (Optional)

None.

@suneox
Copy link
Contributor

suneox commented Feb 14, 2025

I'll take a look on this issue within a few hours

@suneox
Copy link
Contributor

suneox commented Feb 14, 2025

The proposal from @parasharrajat LGTM, and I think we will have a limitation regarding the label overlap with the polyline, which is difficult to resolve since users can zoom in/out. Therefore, I think we can only ensure that the distance label remains visible and as close as possible to the center.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 14, 2025

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

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 15, 2025
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. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants